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

AlecK/APPEALS-44512 - Zeitwerk Autoloader Transition #1700

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

AKeyframe
Copy link
Contributor

@AKeyframe AKeyframe commented Sep 24, 2024

Resolves APPEALS-44512 - Implement Zeitwerk Autoloading for eFolder

🛑🛑🛑 BEFORE DEPLOYMENT

  • Requires AlecK/Zeitwerk to be merged into caseflow-commons.
  • Requires caseflow Gem reference to be updated within the Gemfile.

🛑🛑🛑

Description

This PR makes the necessary changes to switch from classic autoloading to zeitwerk autoloading. This is in preparation for Rails 7.0, where classic autoloading will go from a deprecation warning to fully depreciated.

Acceptance Criteria

  • Running bin/rails zeitwerk:check outputs "All is good!" message.

Testing Plan

@AKeyframe AKeyframe marked this pull request as ready for review October 1, 2024 17:34
@jcroteau jcroteau changed the title Zeitwerk Implementation AlecK/APPEALS-44512 AlecK/APPEALS-44512 - Zeitwerk Autoloader Transition Oct 2, 2024
jcroteau

This comment was marked as resolved.

@jcroteau

This comment was marked as resolved.

@AKeyframe AKeyframe force-pushed the AlecK/APPEALS-44512 branch 2 times, most recently from e8432c8 to 38264e1 Compare October 4, 2024 20:46
jcroteau

This comment was marked as resolved.

@jcroteau jcroteau self-requested a review October 9, 2024 22:30
jcroteau
jcroteau previously approved these changes Oct 9, 2024
AKeyframe and others added 22 commits October 9, 2024 19:41
Co-authored-by: Jeremy Croteau <jcroteau@users.noreply.github.com>
Co-authored-by: Jeremy Croteau <jcroteau@users.noreply.github.com>
Co-authored-by: Jeremy Croteau <jcroteau@users.noreply.github.com>
Co-authored-by: Jeremy Croteau <jcroteau@users.noreply.github.com>
Co-authored-by: Jeremy Croteau <jcroteau@users.noreply.github.com>
Co-authored-by: Jeremy Croteau <jcroteau@users.noreply.github.com>
Co-authored-by: Jeremy Croteau <jcroteau@users.noreply.github.com>
@@ -1,17 +1,18 @@
if ENV["RAILS_ENV"] == "test"
SimpleCov.start do
add_filter "app/controllers/errors_controller.rb"
Copy link

Choose a reason for hiding this comment

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

Is arranging these to be in alphabetical order necessary as a part of this work?

Copy link
Contributor

@jcroteau jcroteau Oct 22, 2024

Choose a reason for hiding this comment

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

This was my doing, not Alec's. Is it necessary? No. But it was a good opportunity to leave something better than I found it, and it's a low-risk change. It was also done as a separate "refactor" commit before adding the new filter.

In general, I think it's a good practice to order things lexically to make them easier to parse (especially if it's a very long list). It also helps to avoid adding duplicate entries for things (which I almost did in this case, and that prompted me to do the reordering).

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.

3 participants