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.

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

Iterate on Java codegen #299

Merged
merged 50 commits into from Dec 3, 2019

Conversation

jmchilton
Copy link
Contributor

@jmchilton jmchilton commented Nov 26, 2019

Build maven project containing Java objects for record types, capable of validation, and that can generate JavaDocs with schema docs contained within.

Missing stuff tracked #303.

@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #299 into master will increase coverage by 2.4%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #299     +/-   ##
=========================================
+ Coverage   59.64%   62.05%   +2.4%     
=========================================
  Files          14       14             
  Lines        2622     2630      +8     
  Branches      727      728      +1     
=========================================
+ Hits         1564     1632     +68     
+ Misses        890      827     -63     
- Partials      168      171      +3
Impacted Files Coverage Δ
schema_salad/codegen_base.py 94.59% <100%> (+14.59%) ⬆️
schema_salad/main.py 56.49% <100%> (+0.49%) ⬆️
schema_salad/codegen.py 91.02% <33.33%> (+74.8%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c83be8...6515752. Read the comment docs.

@tetron
Copy link
Member

tetron commented Nov 26, 2019

One thing you will want to figure out early on is how best to represent sum types in Java, eg a run field can be an embedded Workflow or a CommandLineTool or a ExpressionTool or a URI string. Returning Object and casting everywhere is going to get tedious very quickly.

@jmchilton
Copy link
Contributor Author

Seems like Object would at least be a place to start 😅. The yak I'm shaving with this project currently I think would mostly just benefit from the UnionLoader loading types and verifying things. Validation still happens even if the interface to using the beans is crap.

Still... if I were going to put time into this... hmm... my first thought is it may be worth building UnionOf2<T1, T2>, UnionOf3<T1, T2, T3> .... and UnionOf2Loader, UnionOf3Loader, ....

class UnionOf2<T1, T2> {
   private Object value;
   public Object get();
   private int index; // track type of object by index 
   public boolean is1();
   public boolean is2();
   public T1 get1();
   public T2 get2();
   public Optional<T1> getOptional1();
   public Optional<T2> getOptional2();
}

The type definitions on the beans could become very ugly but may be somewhat informative. Certainly the potential for a lot more type safety also.

Nervous about investing too much time here though.

@lgtm-com
Copy link

lgtm-com bot commented Nov 27, 2019

This pull request introduces 1 alert when merging d23d056 into 4a3f8ca - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

@lgtm-com
Copy link

lgtm-com bot commented Nov 27, 2019

This pull request introduces 2 alerts when merging a04e02d into 4a3f8ca - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Unused import

@tetron
Copy link
Member

tetron commented Nov 27, 2019

@jmchilton you're a beast, thank you so much for working on this

@jmchilton
Copy link
Contributor Author

I got tripped up because schema-salad-tool and codegen modules treat fields with default but not explicitly annotated as optional differently. Probably CWL specs were better about this and so it hasn't been encountered. Eliminating those from my schema - this works! It really validates my relatively complex schema.

@jmchilton
Copy link
Contributor Author

Now with encoded Java formatting (https://github.com/google/google-java-format) and JavaDocs.

Screen Shot 2019-11-27 at 9 47 16 PM

@lgtm-com
Copy link

lgtm-com bot commented Nov 28, 2019

This pull request introduces 4 alerts and fixes 1 when merging daedf25 into 9d1c5e1 - view on LGTM.com

new alerts:

  • 2 for Unused named argument in formatting call
  • 1 for Unused import
  • 1 for Unreachable code

fixed alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Nov 28, 2019

This pull request introduces 3 alerts and fixes 1 when merging ead6ea0 into 9d1c5e1 - view on LGTM.com

new alerts:

  • 2 for Unused named argument in formatting call
  • 1 for Unreachable code

fixed alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Nov 28, 2019

This pull request introduces 3 alerts when merging cfbe9e2 into 4806936 - view on LGTM.com

new alerts:

  • 2 for Unused named argument in formatting call
  • 1 for Unreachable code

@jmchilton
Copy link
Contributor Author

Screen Shot 2019-11-28 at 10 02 36 AM

@mr-c
Copy link
Member

mr-c commented Nov 28, 2019

@jmchilton Super cool! Can you add a main method?

@mr-c
Copy link
Member

mr-c commented Nov 28, 2019

And while I'm making wishes, it would be nice if the schema docs became javadocs too.

I tried your code with both cwl v1.0 and v1.1 's schemas, no crashes :-)

@lgtm-com
Copy link

lgtm-com bot commented Nov 28, 2019

This pull request introduces 3 alerts when merging 7d8c67b into 4806936 - view on LGTM.com

new alerts:

  • 2 for Unused named argument in formatting call
  • 1 for Unreachable code

@jmchilton
Copy link
Contributor Author

A main method that validates a document I assume? Sounds easy enough.

Placing the schema docs in the javadocs is a cool idea! I'm not sure...

@jmchilton
Copy link
Contributor Author

jmchilton commented Nov 28, 2019

I registered the requested validator as the entry point for the jar and I added the maven assembly plugin to build a self contained jar variant that is good for a standalone demo:

$ mvn assembly:single
$ java -jar target/gxformat2-0.0.1-SNAPSHOT-jar-with-dependencies.jar ../test-workflow.yml
Exception in thread "main" org.galaxyproject.gxformat2.gxformat2.utils.ValidationException: Failed to match union type
  Trying 'RecordField'
    the `inputs` field is not valid because:
      Failed to match union type
        Expected object with Java type of java.util.List but got java.util.HashMap
        Trying 'RecordField'
          the `type` field is not valid because:
            Failed to match union type
              Expected null
              Expected one of [File, data, collection]
  Expected object with Java type of java.util.List but got java.util.LinkedHashMap
	at org.galaxyproject.gxformat2.gxformat2.utils.UnionLoader.load(UnionLoader.java:31)
	at org.galaxyproject.gxformat2.gxformat2.utils.Loader.documentLoad(Loader.java:41)
	at org.galaxyproject.gxformat2.gxformat2.utils.RootLoader.loadDocument(RootLoader.java:19)
	at org.galaxyproject.gxformat2.gxformat2.utils.RootLoader.loadDocument(RootLoader.java:85)
	at org.galaxyproject.gxformat2.gxformat2.utils.RootLoader.loadDocument(RootLoader.java:73)
	at org.galaxyproject.gxformat2.gxformat2.utils.RootLoader.loadDocument(RootLoader.java:65)
	at org.galaxyproject.gxformat2.gxformat2.utils.RootLoader.loadDocument(RootLoader.java:31)
	at org.galaxyproject.gxformat2.gxformat2.utils.RootLoader.loadDocument(RootLoader.java:48)
	at org.galaxyproject.gxformat2.gxformat2.utils.Validator.main(Validator.java:12)

@lgtm-com
Copy link

lgtm-com bot commented Nov 28, 2019

This pull request introduces 3 alerts when merging 5a6add1 into 4c83be8 - view on LGTM.com

new alerts:

  • 2 for Unused named argument in formatting call
  • 1 for Unreachable code

@lgtm-com
Copy link

lgtm-com bot commented Nov 28, 2019

This pull request introduces 3 alerts when merging 074722b into 4c83be8 - view on LGTM.com

new alerts:

  • 2 for Unused named argument in formatting call
  • 1 for Unreachable code

@jmchilton
Copy link
Contributor Author

Wasn't my favorite design decision but I resolved a bunch of the type problems by just merging the JavaTypeDef changes back into TypeDef as optional attributes. Fixed most of the remaining type issues.

Added parts of the schema salad docs into the JavaDocs - class, interface, and getters:

Screen Shot 2019-12-01 at 11 40 21 PM

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2019

This pull request introduces 1 alert and fixes 2 when merging f50a87f into 4c83be8 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

fixed alerts:

  • 2 for Unused named argument in formatting call

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2019

This pull request introduces 1 alert and fixes 2 when merging 5841a80 into 4c83be8 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

fixed alerts:

  • 2 for Unused named argument in formatting call

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2019

This pull request introduces 1 alert and fixes 2 when merging 57f9252 into 4c83be8 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

fixed alerts:

  • 2 for Unused named argument in formatting call

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2019

This pull request introduces 1 alert and fixes 2 when merging 284c9e7 into 4c83be8 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

fixed alerts:

  • 2 for Unused named argument in formatting call

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2019

This pull request introduces 1 alert and fixes 2 when merging 7c0ac87 into 4c83be8 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

fixed alerts:

  • 2 for Unused named argument in formatting call

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2019

This pull request introduces 1 alert and fixes 2 when merging e6ca905 into 4c83be8 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

fixed alerts:

  • 2 for Unused named argument in formatting call

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2019

This pull request introduces 1 alert and fixes 2 when merging 6515752 into 4c83be8 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

fixed alerts:

  • 2 for Unused named argument in formatting call

@jmchilton jmchilton changed the title [WIP] Iterate on Java codegen Iterate on Java codegen Dec 2, 2019
"--codegen-target",
type=str,
default=None,
help="Defaults to sys.stdout for python (and ./ for hidden Java codegen)",
Copy link
Member

Choose a reason for hiding this comment

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

Hidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Generate classes in target language, currently supported: python"

I'll add java to the list and remove the word hidden in a follow up.

type=str,
metavar="directory",
default=None,
help="Directory of example documents for test case generation (Java only).",
Copy link
Member

Choose a reason for hiding this comment

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

Is there an example of how to use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/common-workflow-language/schema_salad/blob/master/schema_salad/tests/test_java_codegen.py#L23

I wanted to allow valid and invalid document tests but I never got that far. Currently just assumes all the files in the examples directory are valid documents of this type.

@mr-c
Copy link
Member

mr-c commented Dec 3, 2019

W00t! Shall I merge?

@jmchilton
Copy link
Contributor Author

I have followups in the pipeline - but I'm not touching anything while this is such a pretty shade of green 😆. I'd go ahead an merge now.

@mr-c mr-c merged commit 03a1f23 into common-workflow-language:master Dec 3, 2019
jmchilton added a commit that referenced this pull request Dec 3, 2019
jmchilton added a commit to jmchilton/schema_salad that referenced this pull request Dec 3, 2019
jmchilton added a commit that referenced this pull request Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants