Skip to content

Fix Ruby method parsing for methods containing brackets#3301

Merged
koesie10 merged 1 commit intomainfrom
koesie10/improve-ruby-access-path-parsing
Jan 31, 2024
Merged

Fix Ruby method parsing for methods containing brackets#3301
koesie10 merged 1 commit intomainfrom
koesie10/improve-ruby-access-path-parsing

Conversation

@koesie10
Copy link
Copy Markdown
Member

This fixes parsing of access paths for Ruby methods that contain square brackets, such as Liquid::Context#[]. The previous implementation would incorrectly stop capturing at the ] character, resulting in a method name of [ for an access path of Method[[]].ReturnValue. This fixes it by switching to the shared access path parsing code, which correctly handles square brackets.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

This fixes parsing of access paths for Ruby methods that contain square
brackets, such as `Liquid::Context#[]`. The previous implementation
would incorrectly stop capturing at the `]` character, resulting in a
method name of `[` for an access path of `Method[[]].ReturnValue`. This
fixes it by switching to the shared access path parsing code, which
correctly handles square brackets.
@koesie10 koesie10 marked this pull request as ready for review January 31, 2024 13:19
@koesie10 koesie10 requested a review from a team as a code owner January 31, 2024 13:19
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks! Is it worth adding a test for this?

@koesie10
Copy link
Copy Markdown
Member Author

Thanks! Is it worth adding a test for this?

I agree that more tests may be good, but this is already being tested (implicitly) by these tests: generate.test.ts. I'll merge it for now and we can always add tests later.

@koesie10 koesie10 merged commit e9e9840 into main Jan 31, 2024
@koesie10 koesie10 deleted the koesie10/improve-ruby-access-path-parsing branch January 31, 2024 15:26
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