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

Use reject(&:nil?) in enabled? check #887

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

stevecrozz
Copy link

Rejecting nil has the advantage of allowing use of null object patterns using objects that implement nil? but are not strictly nil. This also works well with the 'actor cannot be nil' guard clause which uses nil? to check nilness.

Just an idea. Would love to hear if there are other ideas. Could handle it differently further down by making Types::Actor.wrap(actor) return nil instead of throw an error. Not sure if that would have unintended affects.

Rejecting nil has the advantage of allowing use of null object patterns using objects that implement nil? but are not strictly nil. This also works well with the 'actor cannot be nil' guard clause which uses `nil?` to check nilness.
@jnunemaker
Copy link
Collaborator

@stevecrozz hey! I have no thoughts. were you just pursuing code or did you hit a snag that required this?

@jnunemaker jnunemaker self-assigned this Nov 7, 2024
@stevecrozz
Copy link
Author

@stevecrozz hey! I have no thoughts. were you just pursuing code or did you hit a snag that required this?

I was inspired by an old RailsConf talk to try using a null object pattern https://www.youtube.com/watch?v=OMPfEXIlTVE

In my data model we have users that may or may not have a company. So without null object, user.company == nil but with null object, user.company becomes an instance of Company::Null. Array#compact won't reject instances of Company::Null.

Probably using compact to reject nils has better performance than reject(&:nil?) but using reject(&:nil?) allows you to implement nil? on your "null object" and it will work.

Ultimately, I don't think I'm going to go through with using this pattern so I'm not actually impacted, but maybe others are, so I wanted to drop this breadcrumb for anyone who might wind up with the same trouble.

I'd be perfectly happy closing this if there's little interest.

@jnunemaker
Copy link
Collaborator

Maybe just add a code comment with link to this PR so I don't forget and switch it (or someone else)? I'd merge after that.

@stevecrozz
Copy link
Author

Sure. I could probably even add a test case so we really don't forget :)

@jnunemaker
Copy link
Collaborator

@stevecrozz haha yeah that's better.

@stevecrozz
Copy link
Author

@jnunemaker I added a test case but it looks like something is up with the Rails7+ scenarios. Looks like an existing issue.

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