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

Fix issues related to passing arguments to funcall and predicates #5539

Merged
merged 2 commits into from
Feb 5, 2018

Conversation

Edouard-chin
Copy link
Contributor

@Edouard-chin Edouard-chin commented Feb 2, 2018

Hey guys, thanks for creating and maintaining this gem 😸
This PR fixes 2 issues I found with the compiler:

Fixed an issue with multiple args on predicates:

  • Any predicate method call was able to receive only a single argument, otherwise an error was thrown during compilation
  • The reason is because the compile_arg method was stumbling into the , token (used to split earch args)

Fixed an issue when funcall were passed argument:

  • Passing argument to funcall (any numbers) wasn't working, the ruby code generate from the compiler wasn't valid, a closing parenthesis was misplaced. The code was looking something like
#before
(my_method(node0), arg1, arg2)

#after
(my_method(node0, arg1, arg2))

@Edouard-chin Edouard-chin force-pushed the predicate-funcall-args branch 3 times, most recently from 7521f07 to 54bbe80 Compare February 2, 2018 02:17
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 4, 2018

The changes look good to me. Just two small notes:

  • Please change "Fixed" to "Fix" in the commit message titles for consistence with commit style
  • Please add some changelog entry for this, as it's a fix in the public API of RuboCop

@Edouard-chin Edouard-chin force-pushed the predicate-funcall-args branch 2 times, most recently from 665f326 to 03e37bd Compare February 4, 2018 22:47
@Edouard-chin Edouard-chin changed the title Fixed issues related to passing arguments to funcall and predicates Fix issues related to passing arguments to funcall and predicates Feb 4, 2018
- Any predicate method call was able to receive only a single argument, otherwise an error was thrown during compilation
- The reason is because the `compile_arg` method was stumbling into the `,` token (used to split earch args)
@Edouard-chin Edouard-chin force-pushed the predicate-funcall-args branch from 03e37bd to 1fa99eb Compare February 4, 2018 22:48
- Passing argument to funcall (any numbers) wasn't working, the ruby code generate from the compiler wasn't valid, a closing parenthesis was misplaced. The code was looking something like
  ```
  # Before
  (my_method(node0), arg1, arg2)

  # After
  (my_method(node0, arg1, arg2))
  ```
@Edouard-chin Edouard-chin force-pushed the predicate-funcall-args branch from 1fa99eb to 8c56b94 Compare February 5, 2018 02:31
@Edouard-chin
Copy link
Contributor Author

Thanks for the quick feedback. I have made the requested changes

@bbatsov bbatsov merged commit fea6e01 into rubocop:master Feb 5, 2018
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 5, 2018

Thanks for working on this! I really appreciate it! 🙇

args
index = tokens.find_index { |token| token == ')' }

tokens.slice!(0..index).each_with_object([]) do |token, args|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it safe to mutate the passed in array here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 , if you are talking about the tokens array then I believe it should be safe to mutate it, well at least the previous implementation was already mutating it

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.

3 participants