-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Introducing zeitwerk #2363
Introducing zeitwerk #2363
Conversation
Change some classes base on file structure Remove all require 'grape/**/*' Remove active_support/autoload Replace eager_load file by zeitwerk eager_load Move and rename Grape.lowercase_headers? to Grape::Http::Headers.lowercase? Add headers_spec conditions before running spec Move lazy_value_*** to their own classes Replace parse_media_type_patch by Grape::Util::Accept (stop overriding Rack:Accept::Header)
Remove validator registration and leverage Zeitwerk
Remove i18n dependency
Remove Coercer collection
What's the downside? |
Follow the file structure convention if we can label that has a "downside". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some comments/asks. Let's increment the version to 2.1.0 as part of this PR since this is not a small change?
Rebase? I'll take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to merge this.
- Update the language around auto/lazy loading/reloading in README.
- What externally visible contracts are we breaking? Should we be incrementing the version to 3.0?
There's a breaking change with custom validator. Now, they must follow the convention |
Sounds good, I'll wait to merge. |
@dblock I've found something else related to gem Next Grape release is |
I don't think they have to. Generally we remove rubies going EOL or if there's "good reason". If you can spell it out in a PR we can do whatever :) |
The code looks good here. I think we still need to say something in UGPRADING/README about auto-loading? WDYT? It feels like a scary change to go "quietly" :) |
@ericproulx Want to add some words to those files and let's merge? |
Done. I mentionned something about Monkey Patch that might result in a Zeitwerk Error. |
Merged, nice work. |
In the past, I tried to make some changes regarding autoload #2200, #2207, #2209. Unfortunately, #2207 and #2209 had to be reverted.
After reading this article, I thought it would be great to use zeitwerk instead of autoload.
Benefits
autoload
eager_load
proofChanges
grape
instead of at compile!eager_load_spec
has been removedvalidator
registration process (were loaded dynamically throughinherited
)