search

dry-rb / dry-inflector

Inflector for Ruby
https://dry-rb.org/gems/dry-inflector
MIT License
95 stars 15 forks source link

Acronyms #22

Closed GustavoCaso closed 6 years ago

GustavoCaso commented 6 years ago

@Ptico @flash-gordon

I started the work on the topic for learning purposes to get familiar with the library and also to have a discussion on how to implement it.

So far I'm not truly happy for storing the acronyms in a Hash.

Todo

Ptico commented 6 years ago

Hi Gustavo, last time I got stuck on the feeling that I missed some use cases. It would be great to have an exhaustive test suite for this feature.

I think it's worth to disturb @abinoam @jodosha and other guys and ask to review the test suite.

GustavoCaso commented 6 years ago

Hi @Ptico, of course, the more the better. I just started with one possible solution and see where it gets me. Please feel free to add any comment you find necessary

AMHOL commented 6 years ago

Don't see the problem with storing them in a hash personally, but probably missing something?

GustavoCaso commented 6 years ago

@AMHOL maybe instead of a simple Hash we could use Concurrent::Hash

AMHOL commented 6 years ago

@GustavoCaso could do, not sure it's necessary as the configuration should probably be done before any forking / threading happens, but no harm in it I guess.

Ptico commented 6 years ago

I think dynamic inflection configuration is a rare case which smells very-very bad. So, plain old hash is ok

flash-gordon commented 6 years ago

OK, I added support for underscore but @Ptico is right, we need more specs, I'll have another look soon. I'm not happy with code but it doesn't matter, we can improve it later once we have all cases covered.

flash-gordon commented 6 years ago

@jodosha heh, it's mostly me, I ported specs and code from activesupport because they're obviously good enough for our case but it's not finished yet, as you pointed docs are missing and I need a fresh look by myself to clean up the resulted implementation (I'm not happy with it atm). I'll get back to it this week, thanks for the input!

flash-gordon commented 6 years ago

Okay, this is good to go, I ended up with creating a separate class for acronyms since they affect different operations and I'm not sure how to unify them within Rules. Anyhow, this doesn't look to be a big deal after all. WRT hashes and potential concurrency issues I think we could turn the whole library into a thread-safe one but this is a separate task because we need to change Rules as well.

I'm merging this with the intent to use acronyms in dry-types, feel free to provide feedback though.