-
Notifications
You must be signed in to change notification settings - Fork 93
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
Record classes are now handled the same as normal classes #1822
Conversation
9a1fcd4
to
9dba2c3
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.
LGTM.
@jukzi would you kindly (review and) merge this? I don't have commit rights :-) |
The code change looks good, but it would be better if you could add a junit test to avoid regression. Can you work on that? |
c03e5fe
to
4f9e796
Compare
This pull request changes some projects for the first time in this development cycle.
An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patch
Further information are available in Common Build Issues - Missing version increments. |
d9a1e13
to
ec081ec
Compare
...e.jdt.text.tests/src/org/eclipse/jdt/text/tests/codemining/ParameterNamesCodeMiningTest.java
Outdated
Show resolved
Hide resolved
...e.jdt.text.tests/src/org/eclipse/jdt/text/tests/codemining/ParameterNamesCodeMiningTest.java
Outdated
Show resolved
Hide resolved
836f2af
to
b595b4c
Compare
@@ -124,6 +124,7 @@ public class Foo { | |||
assertEquals(3, fParameterNameCodeMiningProvider.provideCodeMinings(viewer, new NullProgressMonitor()).get().size()); | |||
} | |||
|
|||
|
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.
redundant new line
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 it!
b595b4c
to
175e16c
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.
LGTM, manually tested, that the test fails without the change.
thanks for contributing.
unrelated build error, i don't know how to handle that |
also 1 Test was added which test the bugfix
175e16c
to
37d127b
Compare
There will be unrelated know 12 fails #1833 that can be ignored today |
so.... merge? :-) |
What it does
Record classes are now handled the same way as normal classes.
This bug fix was proposed here: #1813
Before
Record classes were not checked for the amount of parameters which their methods had. Normal classes where though. That posed the problem, that when only one argument was given, the code minings were shown for record classes but not for normal classes.
How to test
Use this code snippet:
it will result in this:
After the changes
Record classes are checked for the number of parameters the same way as normal classes and methods are checked. For the discussion on when they should be checked please take a look at #1813 (comment) and feel free to tell us your opinion.
Author checklist