-
-
Notifications
You must be signed in to change notification settings - Fork 161
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
Add acceptance tests for keyword arguments #551
Conversation
6e58cad
to
dabf62e
Compare
Even with the check for Ruby version > 2, it seems like the parsing fails. We can wait for the 1.9 drop or remove the offending tests first, what do you prefer @floehopper? I personally would prefer the former 😄
|
I agree that dropping support for Ruby v1.9 first seems like the most sensible option and I should have some availability to make that happen in the next couple of weeks. |
c234c18
to
715fbb5
Compare
715fbb5
to
ddd44a6
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 again for all your work on this. It all looks good to me - I particularly appreciate your thoroughness in adding all these test scenarios.
I've added a couple of small requests inline. Also could you add the information from the PR description into the commit note. I prefer the commit notes to stand on their own and not rely on information in the PRs in case the repo is moved away from GitHub at some point.
Style/BracesAroundHashParameters: | ||
Exclude: | ||
# we specifically want to test hash parameters | ||
- 'test/acceptance/keyword_arguments_test.rb' |
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.
Minor: I think I would prefer this to be disabled in the actual file i.e. around the whole test case if necessary. In my experience, these file-level exclusions tend to get missed/forgotten about over time.
Style/BracesAroundHashParameters: | ||
Exclude: | ||
# we specifically want to test hash parameters | ||
- 'test/acceptance/keyword_arguments_test.rb' |
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.
Minor: I think I would prefer this to be disabled in the actual file i.e. around the whole test case if necessary. In my experience, these file-level exclusions tend to get missed/forgotten about over time.
# with strict keyword matching: | ||
# assert_failed(test_result) |
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 understand why you've done it, but I would prefer not to commit any of these commented-out lines to main
.
# with strict keyword matching: | ||
# assert_failed(test_result) |
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 understand why you've done it, but I would prefer not to commit any of these commented-out lines to main
.
Combined into #554 |
Add some acceptance tests for keyword arguments, somewhat adapted from https://github.com/ruby/ruby/blob/4643bf5d55af6f79266dd67b69bb6eb4ff82029a/doc/NEWS-2.7.0#the-spec-of-keyword-arguments-is-changed-towards-30-.
This is in preparation for supporting strict keyword argument matching, as sketched out in #544 and #446 (comment). These tests will be adjusted subsequently when strict keyword matching is enabled.
TODO: