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

Feature: Mash::UnderscoreKeys Extensions #566

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

arianf
Copy link

@arianf arianf commented Sep 6, 2022

This extension can be mixed into a Mash to change the default behavior of converting keys to be underscore. After mixing this extension into a Mash, the Mash will convert all string keys to underscore. It can be useful to use with external source hashes, which maybe contain hyphens or CamelCase.

class UnderscoreMash < ::Hashie::Mash
  include Hashie::Extensions::Mash::UnderscoreKeys
end
mash = UnderscoreMash.new
mash.updatedAt = 'Today' #=> 'Today'
mash.updatedAt #=> 'Today'
mash[:updated_at] #=> 'Today'
mash['updated_at'] #=> 'Today'
mash.updated_at #=> 'Today'
mash.to_hash #=> {"updated_at"=>true}

The other benefit is hashes that have hyphens can be accessed with methods

mash = UnderscoreMash.new('created-at': 'tomorrow') #=> {"created_at"=>"tomorrow"}
mash.created_at #=> 'tomorrow'

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Couple of questions ... Should we also extend KeyConversion? Find a more generic implementation that can camelize, underscore, etc., keys?


private

def _underscore(string)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I guess I went pretty old school and grabbed the version from 3.1 haha
https://github.com/rails/rails/blob/4dacedf983257aef38a8ebedb2d9a9c8fead8238/activesupport/lib/active_support/inflector/methods.rb#L48

I can update it to reflect the new version, I think there are likely performance and edge case improvements.

The one difference to this method that I added was converting spaces to underscores as well.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we want a different "underscore" behavior than ActiveSupport? Why?

Copy link
Author

@arianf arianf Sep 7, 2022

Choose a reason for hiding this comment

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

Hmm, I was thinking this could be slightly different than Rails. The intent here is making method names compatible with the Ruby's method naming conversions. createdAt -> created_at

But I think it could also go a step further, "created at" also does not follow the method naming convention, as a space is an illegal character. So my thought was we could convert them to underscore as well.

Maybe this should be called Hashie::Extensions::Mash::MethodSafeKeys to not cause confusion. What do you think?

But I'm also okay just keeping it exactly the same as rails. Either way works for me, just feel the former is more convenient

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about implementing a generic extension that lets me transform keys whichever way I want (TransformKeys) as a custom behavior, and then deriving it into a bunch of specific behaviors such as a rails-compatible UnderscoreKeys?

@dangerpr-bot
Copy link

dangerpr-bot commented Sep 6, 2022

1 Error
🚫 One of the lines below found in CHANGELOG.md doesn't match the expected format.
## [Unreleased][unreleased]
### Miscellaneous

Generated by 🚫 Danger

@arianf
Copy link
Author

arianf commented Sep 7, 2022

Should we also extend KeyConversion? Find a more generic implementation that can camelize, underscore, etc., keys?

@dblock were you thinking of Hashie::Extensions::UnderscoreKeys and Hashie::Extensions::CamelCaseKeys as extensions?

@dblock
Copy link
Member

dblock commented Sep 7, 2022

Should we also extend KeyConversion? Find a more generic implementation that can camelize, underscore, etc., keys?

@dblock were you thinking of Hashie::Extensions::UnderscoreKeys and Hashie::Extensions::CamelCaseKeys as extensions?

Yes.

@dblock
Copy link
Member

dblock commented Sep 11, 2022

I think this change is good to merge as is. Other improvements like suggested above can be made later. Can you please fix CHANGELOG to make Danger happy?

@@ -13,6 +13,7 @@ Any violations of this scheme are considered to be bugs.
### Added

* Your contribution here.
* [#566](https://github.com/hashie/hashie/pull/566): Added `Mash::UnderscoreKeys` extensions for conversion of all keys to underscore - [@arianf](https://github.com/arianf)
Copy link
Member

Choose a reason for hiding this comment

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

This is just missing a period.

end

mash = UnderscoreMash.new
mash.updatedAt = 'Today' #=> 'Today'
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Can we lowercase today so that it matches the below tomorrow pls.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looking closer I think ACRONYMS is very controversial. It has nothing to do with underscoring keys, it's a whole other function. Seems like it doesn't belong here. And generally modifying a CONSTANT inside a module is probably not the right way to override anything.

Care to remove for now and let's discuss in a separate PR?

@phortx
Copy link

phortx commented Jan 2, 2023

Any plans to finish this? :)

@mculp
Copy link

mculp commented Jan 30, 2023

me 2

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.

None yet

5 participants