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

Load defaults for Rails 7.2+ #140

Merged
merged 1 commit into from
Jul 7, 2024
Merged

Load defaults for Rails 7.2+ #140

merged 1 commit into from
Jul 7, 2024

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Mar 29, 2024

For #138

load_defaults was added in Rails 5.1: rails/rails#28469

Also, this might be considered a breaking change.

@ankane ankane changed the title Load defaults for Rails 5.1+ Load defaults for Rails 7.2+ Mar 29, 2024
@ankane
Copy link
Contributor Author

ankane commented Mar 29, 2024

Updated to only apply to Rails 7.2+ for backwards compatibility (as suggested in #138).

Edit: Also, sorry for all the noise.

@pat
Copy link
Owner

pat commented Mar 29, 2024

No need to apologise, appreciate you working on this!

I wonder if it's worth releasing this starting from 7.2+ initially (so it's not a breaking change) to confirm things work well, but then consider using it for 5.1+ as a major release/breaking change, and then that opens up removal of a couple of conditionals:

  • lib/combustion/application.rb:28
  • lib/combustion/configurations/active_record.rb:7

Not as many as I'd hoped, though! But still, I think it's worth the breaking change, as verifying we're using default behaviour in each release is a Very Good Idea.

@ankane
Copy link
Contributor Author

ankane commented Mar 29, 2024

Sounds like a plan. fwiw, I've been doing this for a few months for my projects and haven't seen any issues (as one data point).

@pat pat merged commit d41f321 into pat:main Jul 7, 2024
23 checks passed
@pat
Copy link
Owner

pat commented Jul 7, 2024

Finally published this in v1.5.0 - thanks for your patience (and contributions!) ❤️

@ankane
Copy link
Contributor Author

ankane commented Jul 7, 2024

Great, thanks @pat!

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