Skip to content

Conversation

@ericproulx
Copy link
Contributor

@ericproulx ericproulx commented May 9, 2023

This PR replace some custom made functions in favour of ActiveSupport's equivalent.

  • Use ActiveSupport deep_symbolize_keys and deep_merge!
  • Replace object.present? ? object : nil by presence
  • Replace !include? by exclude?
  • Use dup and duplicable? instead of duplicatable? and duplicate

ericproulx added 5 commits May 8, 2023 16:41
…pport functions

Use each_with_object instead of each/inject etc ...
Use deep_dup instead of dup for rack_params
Replace unless present? by if blank?
@ericproulx ericproulx marked this pull request as draft May 9, 2023 16:51
@dblock
Copy link
Member

dblock commented May 9, 2023

🤞 that the ActiveSupport versions perform as well

@ericproulx
Copy link
Contributor Author

ericproulx commented May 10, 2023

🤞 that the ActiveSupport versions perform as well

Should we add a minimum version for active support ? Since we're testing rails 5.2 to 7.0, tests are covering ActiveSupport 5 to 7. Thus, it doesn't test < 5.2. At least we know it works from 5 and beyond :)

@ericproulx
Copy link
Contributor Author

🤞 that the ActiveSupport versions perform as well

Should we add a minimum version for active support ? Since we're testing rails 5.2 to 7.0, tests are covering ActiveSupport 5 to 7. Thus, it doesn't test < 5.2. At least we know it works from 5 and beyond :)

I've restrict to ActiveSupport >= 5. It doesn't work below because of this and we're talking of Ruby 1.9

@dblock
Copy link
Member

dblock commented May 13, 2023

LGTM want to finish this PR?

@ericproulx
Copy link
Contributor Author

I'll run a quick test suites on our codebase to see if there's anything wrong.

@ericproulx ericproulx marked this pull request as ready for review May 14, 2023 14:38
@ericproulx
Copy link
Contributor Author

I'll run a quick test suites on our codebase to see if there's anything wrong.

Everything looks good. There's only the danger part that is failing.

@dblock
Copy link
Member

dblock commented May 14, 2023

Not sure what the Danger problem is, but there's an extra * in the CHANGELOG. Maybe that's it? I can poke it at if you can't figure it out.

@dblock
Copy link
Member

dblock commented May 14, 2023

I think we should either release 1.7.1 before merging this, or make this 1.8.0, wdyt?

@ericproulx
Copy link
Contributor Author

ericproulx commented May 14, 2023

I think we should either release 1.7.1 before merging this, or make this 1.8.0, wdyt?

Good idea, after this PR, I'll open a another that will use ActiveSupport::Deprecation.warn instead of warn [DEPRECATION]. Sticking with ActiveSupport seems the best on the long run.

@dblock
Copy link
Member

dblock commented May 14, 2023

All yours to release/bump!

@ericproulx
Copy link
Contributor Author

All yours to release/bump!

Done. I hope everything is ok. I'm not releasing that often.

@dblock
Copy link
Member

dblock commented May 14, 2023

Let's increment version to 1.8.0 as part of this PR? Feel free to merge after.

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