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

Fixes #460 - Add support for nested generics #1366

Merged
merged 24 commits into from Sep 25, 2018

Conversation

Projects
None yet
2 participants
@pwang347
Contributor

pwang347 commented Sep 18, 2018

Description

The current system does not support (all) nested generics, e.g. Map<List<String>, String>, though Map<String, List<String>> was already supported.

This change aims to ensure that the behaviour is consistent across most cases.

Further note that array conversions will only occur at the outermost layers; for instance, ArrayList<String[]>[] generates List<ArrayList<String[]>> for Java.

@pwang347 pwang347 requested a review from TimLethbridge Sep 18, 2018

@TimLethbridge

Great work.

You will need to figure out why the build is failing and push relevant additional changes to the branch, so they appear in this PR. Other than that, what you are doing looks reasonable. I am curious as to why the Map<String,String> wasn't causing any problems before.

As for your question about validation: Umple doesn't verify the existence or correctness of user-specified types, as that would be too complex. Such problems would need to be detected when the java is subsequently compiled.

pwang347 added some commits Sep 19, 2018

@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Sep 20, 2018

Member

Appveyor is giving this, but wasn't before. Not clear why the error occurs. This same code seems to compile on the other CI platforms and locally.

   [javac] C:\projects\umple\cruise.umple\src-gen-umple\cruise\umple\compiler\StructureDiagramGenerator.java:586: error: method _createJavaScriptCall in class StructureDiagramGenerator cannot be applied to given types;
   [javac]     return this._createJavaScriptCall(0,sb,modelName,bndList).toString(); 
   [javac]                ^
   [javac]   required: Integer,StringBuilder,String,List<ComponentDescriptor>,List<BindingDescriptor>
   [javac]   found: int,StringBuilder,String,List<BindingDescriptor>

The relevant line is supposed to be generated as

return this._createJavaScriptCall(0,sb,modelName,cmpList,bndList).toString(); 

The cmpList list seems to be being left out when it wasn't before.

Member

TimLethbridge commented Sep 20, 2018

Appveyor is giving this, but wasn't before. Not clear why the error occurs. This same code seems to compile on the other CI platforms and locally.

   [javac] C:\projects\umple\cruise.umple\src-gen-umple\cruise\umple\compiler\StructureDiagramGenerator.java:586: error: method _createJavaScriptCall in class StructureDiagramGenerator cannot be applied to given types;
   [javac]     return this._createJavaScriptCall(0,sb,modelName,bndList).toString(); 
   [javac]                ^
   [javac]   required: Integer,StringBuilder,String,List<ComponentDescriptor>,List<BindingDescriptor>
   [javac]   found: int,StringBuilder,String,List<BindingDescriptor>

The relevant line is supposed to be generated as

return this._createJavaScriptCall(0,sb,modelName,cmpList,bndList).toString(); 

The cmpList list seems to be being left out when it wasn't before.

@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Sep 20, 2018

Member

So appveyor is compiling the PR twice. The first compile (using the previous compiler) is OK. The second compile (using the just-compiled-compiler) fails. This suggests that the changes in the PR are at fault and it is not a configuration issue in Appveyor.

Member

TimLethbridge commented Sep 20, 2018

So appveyor is compiling the PR twice. The first compile (using the previous compiler) is OK. The second compile (using the just-compiled-compiler) fails. This suggests that the changes in the PR are at fault and it is not a configuration issue in Appveyor.

@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Sep 20, 2018

Member

@pwang347 I can confirm that the Appveyor errors are a result of this PR and not a configuration problem. Appveyor does two compiler builds in succession, and the second one fails. In your branch on my machine I can replicate that by simply doing who quick builds in succession. See my other comments above. But the problem does not occur in master.

Member

TimLethbridge commented Sep 20, 2018

@pwang347 I can confirm that the Appveyor errors are a result of this PR and not a configuration problem. Appveyor does two compiler builds in succession, and the second one fails. In your branch on my machine I can replicate that by simply doing who quick builds in succession. See my other comments above. But the problem does not occur in master.

@pwang347

This comment has been minimized.

Show comment
Hide comment
@pwang347

pwang347 Sep 20, 2018

Contributor

@TimLethbridge Thanks for this! Just curious, are the build definitions in source control as well? I can see the script used for TravisCI but not for Appveyor it seems.

Contributor

pwang347 commented Sep 20, 2018

@TimLethbridge Thanks for this! Just curious, are the build definitions in source control as well? I can see the script used for TravisCI but not for Appveyor it seems.

@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Sep 20, 2018

Member

@yES, The build definitions are in source control. The appveyor one is appveyor.yml at the root

Member

TimLethbridge commented Sep 20, 2018

@yES, The build definitions are in source control. The appveyor one is appveyor.yml at the root

pwang347 added some commits Sep 21, 2018

aie
@TimLethbridge

This comment has been minimized.

Show comment
Hide comment
@TimLethbridge

TimLethbridge Sep 25, 2018

Member

Great job

Member

TimLethbridge commented Sep 25, 2018

Great job

@TimLethbridge TimLethbridge merged commit 7840197 into master Sep 25, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@pwang347 pwang347 deleted the issue_460 branch Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment