-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixes #2347 - Correct full path building for lateral scopes #2469
Conversation
60ec116
to
31ce885
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! Forgive me for nitpicking.
CHANGELOG.md
Outdated
@@ -8,6 +8,7 @@ | |||
|
|||
* [#2467](https://github.com/ruby-grape/grape/pull/2467): Fix repo coverage - [@ericproulx](https://github.com/ericproulx). | |||
* [#2468](https://github.com/ruby-grape/grape/pull/2468): Align `error!` method signatures across different places - [@numbata](https://github.com/numbata). | |||
* [#2469](https://github.com/ruby-grape/grape/pull/2469): Fixes #2347 - Correct full path building for lateral scopes - [@numbata](https://github.com/numbata). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s say “Fix” to match other lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! (diff)
@@ -190,7 +190,10 @@ def push_declared_params(attrs, **opts) | |||
# | |||
# @return [Array<Symbol>] the nesting/path of the current parameter scope | |||
def full_path | |||
nested? ? @parent.full_path + [@element] : [] | |||
return (@parent.full_path + [@element]) if nested? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike the double return, can we do
if
…
elsif
…
else
…
then we can omit the returns altogether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it was my first implementation (with "if-else"), but then rubocop shamed me for huge amount of lines of class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually just do rubocop -a ; rubocop --auto-gen-config
.
58d54c3
to
cc54313
Compare
Should fix the #2347 issue.
This pull request fixes an issue where the full_path method would stop building the full path when encountering lateral scopes like
with
orgiven
. The updated method now correctly handles these cases, ensuring accurate path constructionThanks @seriousdev-gh for the spec.