Skip to content

Remove unused Request::DEFAULT_PARAMS_BUILDER constant #2556

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

Merged
merged 2 commits into from
Apr 11, 2025

Conversation

eriklovmo
Copy link
Contributor

The constant was introduced in 45abe0d but is never referenced and hasn't been since its inception.

The constant was introduced in 45abe0d
but is never referenced and hasn't been since its inception.
@grape-bot
Copy link

grape-bot commented Apr 10, 2025

1 Warning
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by 🚫 Danger

@ericproulx
Copy link
Contributor

ericproulx commented Apr 10, 2025

The constant was introduced in 45abe0d but is never referenced and hasn't been since its inception.

So true, I was supposed to use it but I reused the Grape.config.param_builder. Thanks for pointing out

@eriklovmo
Copy link
Contributor Author

Do we want a changelog entry for this?

@ericproulx
Copy link
Contributor

Do we want a changelog entry for this?

Please, even though its seems insignificant.

@dblock
Copy link
Member

dblock commented Apr 10, 2025

This was shipped in a previous version? I think we want a CHANGELOG and UPGRADING that mentions the removal of the constant since it is public and could have been used.

@eriklovmo
Copy link
Contributor Author

eriklovmo commented Apr 11, 2025

Do we want a changelog entry for this?

Please, even though its seems insignificant.

Updated the changelog in 8b09187. @ericproulx

This was shipped in a previous version? I think we want a CHANGELOG and UPGRADING that mentions the removal of the constant since it is public and could have been used.

The constant we're removing here was introduced after version 2.3.0, so it hasn’t been officially released yet. For that reason, I’m thinking we shouldn't mention it in the UPGRADING doc. @dblock

@dblock
Copy link
Member

dblock commented Apr 11, 2025

The constant we're removing here was introduced after version 2.3.0, so it hasn’t been officially released yet. For that reason, I’m thinking we shouldn't mention it in the UPGRADING doc. @dblock

Yep, but also NBD.

@dblock dblock merged commit a80d2d4 into ruby-grape:master Apr 11, 2025
63 checks passed
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.

4 participants