Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: IListEmpty and friends #76

Merged

Conversation

LowLevelSubmarine
Copy link
Contributor

Fix for #75

@LowLevelSubmarine
Copy link
Contributor Author

I've just added the other Empty types aswell, do you have some suggestion what i could add to the current tests? @marcglasberg

@LowLevelSubmarine LowLevelSubmarine marked this pull request as ready for review March 27, 2024 16:59
@LowLevelSubmarine
Copy link
Contributor Author

LowLevelSubmarine commented Mar 27, 2024

Two things:
Whats your opinion on overriding isEmpty / isNotEmpty on the Empty types like so:

  /// An empty list is always empty, by definition
  @override
  bool get isEmpty => true;
  
  /// An empty list is always empty, by definition
  @override
  bool get isNotEmpty => false;

Do you think we should discourage or even disallow instantiating the IListEmpty directly instead of using IList.empty()?
Although it is one char longer, i see the benefit of automatic type coercion beeing IList and not IListEmpty:

var ilist1 = IList.empty() // is of type IList
var ilist2 = IListEmpty() // is of type IListEmpty

I do not see the benefit of having a variable of type FooEmpty, users will always want to reassign the list if they declare an empty ilist right?

@marcglasberg
Copy link
Owner

Yes, you should override all methods that now have trivial implementations, including isEmpty etc. This will make it faster.

And I also see no point in instantiating IListEmpty directly, if you can simply write const ilist1 = IList.empty() and get an IList.

@LowLevelSubmarine
Copy link
Contributor Author

What do you think can we do to discourage instantiating IListEmpty()? We could make the constructors private

@marcglasberg
Copy link
Owner

Yes, sure, make them private.

@LowLevelSubmarine
Copy link
Contributor Author

I did all the changes we've discussed, you can merge now if you want

@marcglasberg marcglasberg merged commit 35ddd1b into marcglasberg:master Mar 27, 2024
1 check passed
@marcglasberg
Copy link
Owner

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants