8000 Add generated code for cwl-v1.2.1_proposed by ZimmerA · Pull Request #2 · common-workflow-lab/cwl-ts-auto · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add generated code for cwl-v1.2.1_proposed #2

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
Merged
merged 19 commits into from
Dec 23, 2021
Merged

Add generated code for cwl-v1.2.1_proposed #2

merged 19 commits into from
Dec 23, 2021

Conversation

ZimmerA
Copy link
Collaborator
@ZimmerA ZimmerA commented Dec 9, 2021 •
edited
Loading

This is the first version of cwl-ts-auto generated by the codegen currently under development in common-workflow-language/schema_salad#479 using the cwl-v1.2.1_proposed schema

Any bug reports or improvement suggestions are highly appreciated!
Keep in mind that this code currently only has been tested with node.js v17 and probably does not work in the browser yet.

If there are any questions, feel free to ask in here or contact me privately. I will try to upload examples on how to use this library shortly!

TODO to merge this:

@ZimmerA ZimmerA mentioned this pull request Dec 13, 2021
Closed
@mr-c
Copy link
Member
mr-c commented Dec 13, 2021

Great to see the progress! Any thoughts on how to wire up the tests to run here?

@ZimmerA
- Add more tests
50091ce 
- Fix bug that caused exceptions not getting thrown
- Fix loadDocumentByString tests baseuri parameter
@ZimmerA
- Prefix errors variable with __ to reduce name collision
46ebf12 
- Refactor example tests
@ZimmerA
Copy link
Collaborator Author
ZimmerA commented Dec 13, 2021 •
edited
Loading

Great to see the progress! Any thoughts on how to wire up the tests to run here?

Yeah, I was going to copy over the workflow from #1 I just didn't get around to doing it yet 😸

@codecov-commenter
Copy link
codecov-commenter commented Dec 13, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@5abb127). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main       #2   +/-   ##
=======================================
  Coverage        ?   33.54%           
=======================================
  Files           ?      126           
  Lines           ?     6371           
  Branches        ?     1569           
=======================================
  Hits            ?     2137           
  Misses          ?     3939           
  Partials        ?      295
Flag Coverage Δ
unittests 33.54% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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 5abb127...8e695bf. Read the comment docs.

@ZimmerA ZimmerA mentioned this pull request Dec 17, 2021
Open
@ZimmerA
Add interfaces for document properties
14ec7fd
@ZimmerA
List of changes:
99b3b42 
- Change file naming convention to all lower case
- Introduce enum types for enum fields (instead of runtime checked strings)
- Add default parameter value  for "class" fields
@ZimmerA
Copy link
Collaborator Author
ZimmerA commented Dec 20, 2021

Change file naming convention to all lower case\

Aww, that is a bummer. Is that a Typescript thing?

Heh, there isn't really a convention on this in ts besides "keep it consistent". I would personally prefer camelCase.ts filenames, but it's hard to autogenerate for class names like CWLVersion, because you would need to know that CWL is an acronym and the 'V' of version has to be capitalized again. Of course, you could add acronyms like CWL and the name conversion function would check against those, but this would get broken by any new acronyms. If you have any idea, let me know!

@mr-c
Copy link
Member
mr-c commented Dec 20, 2021 •
edited
Loading

Change file naming convention to all lower case\

Aww, that is a bummer. Is that a Typescript thing?

Heh, there isn't really a convention on this in ts besides "keep it consistent". I would personally prefer camelCase.ts filenames, but it's hard to autogenerate for class names like CWLVersion, because you would need to know that CWL is an acronym and the 'V' of version has to be capitalized again. Of course, you could add acronyms like CWL and the name conversion function would check against those, but this would get broken by any new acronyms. If you have any idea, let me know!

Why not keep the case as-is? If people want another case convention than the one used in their source schema salad documents, they can apply a post-codegen transformation, yes?

@ZimmerA
Copy link
Collaborator Author
ZimmerA commented Dec 20, 2021 •
edited
Loading

Why not keep the case as-is? If people want another case convention than the one used in their source schema salad documents, they can apply a post-codegen transformation, yes?

That would break consistency with the filenames in the util folder. Other than that, I agree.

That aside, this is just the file names, the types and classes itself will keep their original name and will also be imported with the original name. The library consumer will never be concerned with the file names.
Example for commandlinetool.ts:

import { CommandLineTool } from 'cwl-ts-auto'

Do you think it would still be better to have the file names as is? I am not attached to the 'all lower case' standard 😄

Edit: I overread the post-codegen transformation part, that could be a good solution!

@ZimmerA
Undo all lowercase file names
df05c42
@ZimmerA
List of changes:
9528c8d 
- Fix 'undefined' typeguard
- Fix comparison in saveRelativeUri
- Add missing docRoot parameter to loader functions
@mr-c
Copy link
Member
mr-c commented Dec 23, 2021

@ZimmerA shall we merge this and cut a 0.1 release?

@ZimmerA
Copy link
Collaborator Author
ZimmerA commented Dec 23, 2021

@ZimmerA shall we merge this and cut a 0.1 release?

Can do, I will prepare the release and then we can merge 👍🏻

@ZimmerA
Copy link
Collaborator Author
ZimmerA commented Dec 23, 2021

@mr-c I would say this is ready, what do you think?

@ZimmerA ZimmerA marked this pull request as ready for review December 23, 2021 13:41
@mr-c
Copy link
Member
mr-c commented Dec 23, 2021

Go for it!

@mr-c
Copy link
Member
mr-c commented Dec 23, 2021

Maybe mention that this is only for CWL v1.2 syntax, and other documents will need upgrading using https://pypi.org/project/cwl-upgrader/ first?

@ZimmerA ZimmerA merged commit 142cd1d into common-workflow-lab:main Dec 23, 2021
@ZimmerA
Copy link
Collaborator Author
ZimmerA commented Dec 23, 2021

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ZimmerA @mr-c @codecov-commenter
0