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.

Sign up for GitHub

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

Jump to bottom

#10 - [WARNING] Attempt to create a file for type 'package.to.MyEditor_field_Context' multiple times when running tests #16

Merged
merged 5 commits into from
Jul 8, 2021
Merged

#10 - [WARNING] Attempt to create a file for type 'package.to.MyEditor_field_Context' multiple times when running tests #16

merged 5 commits into from
Jul 8, 2021

Conversation

FrankHossfeld
Copy link
Member

No description provided.

@FrankHossfeld
fixing several issues:
da15dbf 
gwtproject#10 - [WARNING] Attempt to create a file for type 'package.to.MyEditor_field_Context' multiple times when running tests
@FrankHossfeld
fixing several issues:
10a3253 
gwtproject#10 - [WARNING] Attempt to create a file for type 'package.to.MyEditor_field_Context' multiple times when running tests - fixed formating
niloc132
niloc132 reviewed Mar 6, 2021
View reviewed changes
gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated Show resolved Hide resolved
gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated Show resolved Hide resolved
gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated
ClassName.get(((DeclaredType) data.getEditorType()).getTypeArguments().get(1))),
"editor",
Modifier.PRIVATE);
} else if (((DeclaredType) data.getEditorType()).getTypeArguments().size() == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic is a bit gross, wonder if we can generalize instead of handling different arity like this...

in all three cases we add a field, the type of the field just varies. Zero type args is a special case, where we just make it "raw" apparently, though raw seems actually wrong to me? It should actually be unparamaterized (but maybe the variable is just poorly named).

In the other two cases, i wonder if we could just stream the type args to pass to ParameterizedTypeName.get(baseType, args...), make them an array after doing the logic on them? It would support super-weird cases like 3 or more args, and it might even be shorter than doing it by hand twice...

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider CompositeEditor as an example of a case with three args

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yes, looks like Script Kid Programming .... Think, found a better solution.

gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated Show resolved Hide resolved
gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated
@@ -307,8 +345,8 @@ private ClassName getEditorDelegate(EditorModel editorModel, EditorProperty data
.addModifiers(Modifier.PROTECTED)
.returns(void.class)
.addAnnotation(Override.class)
.addParameter(Editor.class, "editor")
.addStatement("this.editor = ($T) editor", rawEditorType)
.addParameter(ClassName.get(data.getEditorType()), "editor")
Copy link
Contributor

Choose a reason for hiding this comment

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

does this ever need to be parameterized?

Copy link
Member Author

@FrankHossfeld FrankHossfeld Mar 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I moved back to the original implementation, cause we have another issue regarding the generation warning issue. (#11) Might be the better place to discuss it.

This will lead to more ' missing type arguments`-warnings, but will not create compilation errors.

gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated
@@ -324,8 +362,8 @@ private ClassName getEditorDelegate(EditorModel editorModel, EditorProperty data
.addModifiers(Modifier.PROTECTED)
.returns(void.class)
.addAnnotation(Override.class)
.addParameter(ClassName.get(Object.class), "object")
.addStatement("this.object = ($T) object", ClassName.get(data.getEditedType()))
.addParameter(ClassName.get(data.getEditedType()), "object")
Copy link
Contributor

Choose a reason for hiding this comment

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

so as above, is there any chance that this could go wrong if the edited type is itself generic?

Copy link
Member Author

Choose a reason for hiding this comment

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

see a answer above

gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated Show resolved Hide resolved
gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated Show resolved Hide resolved
gwt-editor-processor/src/main/java/org/gwtproject/editor/processor/DriverProcessor.java Outdated Show resolved Hide resolved
...editor-processor/src/test/resources/org/gwtproject/editor/processor/test08/TestEditor08.java Outdated Show resolved Hide resolved
niloc132
niloc132 reviewed Jul 8, 2021
View reviewed changes
gwt-editor-processor/pom.xml Outdated
<auto.common.version>0.10</auto.common.version>
<auto.service.version>1.0-rc6</auto.service.version>
<auto.common.version>0.11</auto.common.version>
<auto.service.version>1.0-rc7</auto.service.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

1.1 and 1.0 are latest of these now

niloc132
niloc132 reviewed Jul 8, 2021
View reviewed changes
...ces/org/gwtproject/editor/processor/test08/result/TestEditor08_SimpleBeanEditorDelegate.java
import org.gwtproject.editor.client.testing.FakeLeafValueEditorWithHasEditorDelegate_Long_SimpleBeanEditorDelegate;
import org.gwtproject.editor.client.testing.FakeLeafValueEditorWithHasEditorDelegate_String_SimpleBeanEditorDelegate;
import org.gwtproject.editor.processor.common.Model01Dto;
public class TestEditor08_SimpleBeanEditorDelegate extends SimpleBeanEditorDelegate {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we're still missing generics here

Copy link
Member Author

Choose a reason for hiding this comment

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

correct ... that is another upcoming PR

niloc132
niloc132 reviewed Jul 8, 2021
View reviewed changes
Copy link
Contributor

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Let's fix the versions and ship

@FrankHossfeld
Update pom.xml
ab79be1 
changed version of auto-common to 1.1 & auto-version to 1.0
@niloc132 niloc132 merged commit ebb2d57 into gwtproject:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niloc132 niloc132 niloc132 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FrankHossfeld @niloc132