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

Allow initializing enummer with hash #22

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

GabrielNagy
Copy link
Contributor

Similar to the Rails enum behavior, allow initializing an enummer with a hash where the values correspond to the indices of the bits that map to the flag.

This is a more robust and backwards-compatible approach to enums, as the developer doesn't have to worry about migrating historical data if an enum value is removed.

I also took the liberty of running standardrb on the entire project, seeing as the gem is part of the development dependencies.

Thanks again for this useful gem!

@shkm
Copy link
Owner

shkm commented Aug 13, 2024

Hi and thanks for the PRs! I'll definitely give them a look as soon as I have a spare moment!

@shkm
Copy link
Owner

shkm commented Aug 26, 2024

Hey hey! Would you mind looking at the conflicts, @GabrielNagy?

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (6fd34bf) to head (b051e9e).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #22   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines           70        71    +1     
  Branches         8         9    +1     
=========================================
+ Hits            70        71    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Similar to the Rails `enum` behavior, allow initializing an `enummer`
with a hash where the values correspond to the indices of the bits that
map to the flag.

This is a more robust and backwards-compatible approach to enums, as the
developer doesn't have to worry about migrating historical data if an
enum value is removed.
@GabrielNagy
Copy link
Contributor Author

Hey hey! Would you mind looking at the conflicts, @GabrielNagy?

All done, thanks!

Support for using strings as setters was added in 441cdf6, but the
initial implementation suffers from an idempotency issue, as changes
would be committed to the DB even if the values were the same. Consider
the following scenario:

    u = User.last
    => #<User:0x00007c747d137810 id: 3, permissions: [:read, :write], facial_features: [], diets: [], transport: [], home: []>

    u.permissions = u.permissions.map(&:to_s)
    => ["read", "write"]

    u.changes
    => {"permissions"=>[[:read, :write], ["read", "write"]]}

Because we cast the values to symbols when serializing, we set the
correct bitmask but this comes with the drawback of altering the DB
even if the new values are the same.

A better way to handle this issue is to delegate the responsibility to
the `cast` method. If we do this we can drop the initial fix, as values
will _always_ be symbols.

It's tricky to test something like this, so I've opted to hook into
ActiveRecord notifications and confirm no SQL UPDATE is being triggered
if the values are the same.
@GabrielNagy
Copy link
Contributor Author

I've pushed an additional commit to fix another issue I ran into while using enummer. It's completely unrelated to the functionality introduced by the PR at hand but I added it here to avoid conflicts and such.

Let me know if you want me to open a separate PR for it @shkm. Thanks!

Copy link
Owner

@shkm shkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work. Thank you so much for this!

Apologies again for the delay; I got hit with COVID (all good now :-)) and have been otherwise busy since!

@shkm shkm merged commit 6a5155f into shkm:main Sep 12, 2024
12 checks passed
@GabrielNagy GabrielNagy deleted the values-with-indexes branch September 12, 2024 10:27
@GabrielNagy
Copy link
Contributor Author

Cheers! Thanks for cutting a new release too 🎉

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