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

Remove more deprecations #93

Merged
merged 10 commits into from
Nov 20, 2023
Merged

Remove more deprecations #93

merged 10 commits into from
Nov 20, 2023

Conversation

etiennebarrie
Copy link
Member

I forgot a few in #90, this removes them.

I also had to bring the new "fake Rails 7.1 app" setup so that we wouldn't call the deprecated behavior. And similarly kept the tests testing that behavior but only when not running Active Support >= 7.1.

We paired with @adrianna-chang-shopify on this. We had to extract the RSpec plugin setup so that we could call it and check its behavior, previously the plugin specs were not exercising anything, just asserting that the behavior had happened while booting the spec suite.

Also CI was mistakenly configured where it was testing 7.1 with gemfiles/activesupport_7.0.gemfile Gemfile. And we were not testing with Gemfile directly.

@@ -16,6 +16,7 @@ def with_rails_70_app
with_rails_70_app do
behavior = ActiveSupport::Deprecation::DEFAULT_BEHAVIORS[:notify]

DeprecationToolkit::RSpecPlugin.before_suite
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we should move this to the with_rails_70_app helper actually? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I like having the test itself doing the exercise step if possible.

@etiennebarrie etiennebarrie merged commit c374f14 into main Nov 20, 2023
51 checks passed
@etiennebarrie etiennebarrie deleted the remove-more-deprecations branch November 20, 2023 11:01
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