[Fix #903] Fix deprecation warning in LogSubscriber in Rails 7.1

toptal / chewy

High-level Elasticsearch Ruby framework based on the official elasticsearch-ruby client

MIT License

1.88k stars 366 forks source link

[Fix #903] Fix deprecation warning in LogSubscriber in Rails 7.1 #907

Closed alejandroperea closed 9 months ago

alejandroperea commented 9 months ago

Fixes https://github.com/toptal/chewy/issues/903

color method was changed in Rails 7.1, introducing a mode_options hash instead of a position parameter just for bold option.

This PR checks the ActiveSupport version and calls color method with the proper parameters.

Also, I've added the configuration for executing the CI for Rails 7.1 version

Before submitting the PR make sure the following are checked:

[ ] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
[ ] Wrote good commit messages.
[ ] Commit message starts with [Fix #issue-number] (if the related issue exists).
[ ] Feature branch is up-to-date with master (if not - rebase it).
[ ] Squashed related commits together.
[ ] Added tests.
[ ] Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

konalegi commented 9 months ago

Great, thank you for the PR, could you please update the change log as well? Thank you! Something like this https://github.com/toptal/chewy/pull/892/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR7. Once CI pass, I guess I'm fine with merge.

alejandroperea commented 9 months ago

@konalegi I've squashed the changes with the CHANGELOG.

The 7.1 builds were failing because of this:

https://github.com/toptal/chewy/pull/907/files#diff-de55eb9d876f7c9af5c2ed3ad73de1336e1ceadd73284015bfccf5d435e58de3R3

As it was unable to require active_support/deprecation. I don't know if this is OK for now (as chewy it is not using any deprecation). Or maybe is it better to remove it and rethink it when chewy needs to add a deprecation.

Let me know what you think 👍

konalegi commented 9 months ago

Let me know what you think 👍 It's fine as long as we need to support different versions and CI is green (In CI we trust).

But please, fix a minor issue that I've commented in your PR

alejandroperea commented 9 months ago

@konalegi Changes done 👍 I think you have to trigger the workflows. Let's see what happens

alejandroperea commented 9 months ago

It doesn't work and makes total sense, as it is needed to load the deprecators, depending the version one or another.

If today I have time, I will take a deeper look and come back to you.

alejandroperea commented 9 months ago

@konalegi I was looking for the same error in other gems. Based on what CocoaPods did and the official documentation, I believe requiring the bare minimum with require 'active_support' and then the extensions needed, it fixes the problem.

konalegi commented 9 months ago

@alejandroperea I'll take a look, I don't know what exactly adds require 'active_support' don't really want to include some extra stuff, that we don't really need. I'll take a look in 4-5 hours.

alejandroperea commented 9 months ago

Sure @konalegi , no hurries! What I understand from the documentation, it only loads the bare minimum and some entry points for being able to load then the needed extensions, but not 100% sure.

Now the tests are passing. Let me know if you need further changes or investigation 👍

konalegi commented 9 months ago

I'll try to gather 2nd review, if not, will merge later.

alejandroperea commented 9 months ago

Thanks for checking it @konalegi !

konalegi commented 9 months ago

released in https://rubygems.org/gems/chewy/versions/7.3.5

alejandroperea commented 9 months ago

Thank you so much @konalegi !