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

Add ActiveSupport::Inflector to Zeitwerk #793

Conversation

jclusso
Copy link
Contributor

@jclusso jclusso commented Aug 9, 2023

This is a 🙋 feature or enhancement.
This is a 🔦 documentation change.

Summary

To make things easier I've made it so Zeitwerk uses ActiveSupport::Inflector out of the box. I'm not really sure how to add tests for this. Looking for assistance on that if required to merge this. I've testsed this in my own project and it works.

Changes

  • Configure Zeitwerk's inflector to use ActiveSupport::Inflector
  • Add commented out example to config/initializer.rb
  • Update existing documentation about ActiveSupport::Inflector to reference initializer configuration
  • Add documentation for initializer

jclusso added 2 commits August 9, 2023 00:36
- Configure Zeitwerk's inflector to use ActiveSupport::Inflector
- Add commented out example to config/initializer.rb
- Update existing documentation about ActiveSupport::Inflector to reference initializer configuration
- Add documentation for initializer
@jaredcwhite
Copy link
Member

Thanks @jclusso, looks good. I need to think about the best way to merge this is as it is a breaking change (I don't want folks to upgrade from v1.3 to 1.4 and suddenly various classes break). We may want to keep it opt-in only at the start, and then switch to default in v2.0. What do you think?

@jclusso
Copy link
Contributor Author

jclusso commented Aug 20, 2023

@jaredcwhite yeah, I didn't consider that it could be breaking. I think making it opt-in until v2 seems reasonable. Any suggestions on how you'd think we best do that?

@jclusso
Copy link
Contributor Author

jclusso commented Aug 21, 2023

I had some more thought on this, but not sure how you'd like to proceed. Instead of changing this PR, we can leave this here until v2.0 is ready and you can merge it then since I believe this is ready for that from a code and documentation perspective.

To make it optional, I could make another PR where we add autoload_inflector to Bridgetown::Configurationand I can update the documentation and initializer to set it.

@jaredcwhite
Copy link
Member

@jclusso That last suggestion sounds good to me. If it could be a configuration option to enable it, I'm all for it.

@jclusso
Copy link
Contributor Author

jclusso commented Aug 22, 2023

Replaced by #796 and #797

@jclusso jclusso closed this Aug 22, 2023
@jclusso jclusso deleted the add_active_support_inflector_to_zeitwerk branch August 22, 2023 04:54
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