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

Fixes #2347 - Correct full path building for lateral scopes #2469

Merged
merged 3 commits into from
Jul 7, 2024

Conversation

numbata
Copy link
Contributor

@numbata numbata commented Jul 4, 2024

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 or given. The updated method now correctly handles these cases, ensuring accurate path construction

Thanks @seriousdev-gh for the spec.

@numbata numbata changed the title Fixes #2347 The building full_path for the lateral scopes Fixes #2347 - Correct Path Building for Lateral Scopes Jul 4, 2024
@numbata numbata changed the title Fixes #2347 - Correct Path Building for Lateral Scopes Fixes #2347 - Correct full path building for lateral scopes Jul 4, 2024
@numbata numbata marked this pull request as ready for review July 4, 2024 14:47
Copy link
Member

@dblock dblock left a 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).
Copy link
Member

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.

Copy link
Contributor Author

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?
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed here

Copy link
Member

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.

@dblock dblock merged commit da9815d into ruby-grape:master Jul 7, 2024
52 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.

2 participants