fix: printing of generic records (issue #4281) #4283
Conversation
There was a problem hiding this comment.
Hi @xzel23 !
Thank you for the pull request. Please look at my comment.
EDIT: Sorry, I wasn't aware about the record feature in Java. Let me have a look again.
| @@ -0,0 +1,3 @@ | |||
| package records; | |||
|
|
|||
| public record GenericRecord<T>(T a, T b) {} | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
It looks good to me. Thanks, @xzel23 ! I have also suggested some changes so please see the comments for that. I am sorry I gave a misinformed review first.
Anyway, @monperrus @slarse , you can also have a look.
| package records; | ||
|
|
||
| public record GenericRecord<T>(T a, T b) {} |
There was a problem hiding this comment.
Could you please add this test case under src/test/resources/records? It's where we put all the tests related to records.
There was a problem hiding this comment.
Hm... I think the suggested path is the same as where the file is currently located. But I think I should update the package name.
| @@ -146,6 +146,16 @@ private CtModel createModelFromPath(String code) { | |||
| return model; | |||
| } | |||
|
|
|||
| @Test | |||
| void printGenericRecord() { | |||
There was a problem hiding this comment.
We try to name new tests we create by describing what is happening in the test. I think a better name would be testGenericTypeParametersArePrintedBeforeTheFunctionParameters.
There was a problem hiding this comment.
OK, will update the PR
|
@algomaster99 Yes, I have seen code in Spoon using "record" as a variable name in the test classes. That code will probably break when compiling in Java 14+ where it was introduced as a keyword. The code in the unit test actually is more or less taken from one of my source files that compile perfectly but cannot be handled by spoon. |
@xzel23 We don't compile our test resources before using them in tests and probably that's why all the tests are green when executed in ubuntu/Java 16 environment. Should we consider compiling our test resources, @slarse? We do it in
Spoon doesn't compile code before building the model. You can even create a model for syntactically incorrect code. For example, public class A {
x;
}The return type of field is missing. However, we guarantee correct transformations only for syntactically correct code. What you encountered was a bug in the printer (thanks for the fix). In other words, spoon created a valid model for your code. It just failed to print it correctly when you called |
|
@algomaster99 I just learned that using |
Oh, nice. Our test resources are free of at least one possible error :) Thanks for renaming the test. Could you also shift the resource to the suggested directory? |
|
@algomaster99 I would do it, but I'm afraid I can't ;-) Please compare the paths. First one is the current resource path, second one the suggested directory:
I don't see any difference. Where should I move the file to? |
|
Oh, right. I mistook the path because of diffs in multiple file. 😅 I think we are good to go. Let maintainers approve your PR for workflow and then we will review further, if needed. |
| @@ -146,6 +146,16 @@ private CtModel createModelFromPath(String code) { | |||
| return model; | |||
| } | |||
|
|
|||
| @Test | |||
| void testGenericTypeParametersArePrintedBeforeTheFunctionParameters() { | |||
| // a record with generic type arguments should be printed correctly | |||
There was a problem hiding this comment.
Could you add a // contract: ... a record with generic type arguments should be printed correctly?
This is by design, a lot of the test resources can't compile as they are missing dependencies. It would however be neat to check the syntax. There's already something along the lines of a syntax check implemented in Approved the workflow, getting back to this later. |
|
@MartinWitt you approved with a comment, are you happy with the PR? if so I merge. |
|
yes you can merge. |
|
@MartinWitt okidoki Thanks @xzel23 for the fix! |
Fix #4281
This PR moves the type argument to the correct location when printing generic record declarations. A new unit test is included.