Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merge argparse2cwl + gxargparse #32

Merged
merged 62 commits into from
Nov 15, 2016
Merged

Conversation

anton-khodak
Copy link
Collaborator

brainstorm and others added 30 commits December 14, 2015 10:53
Add section for CWL tool wrapper
Improved templates, add more actions and params, add output section
Improved CLI, templates, output section, first test
@hexylena
Copy link
Owner

hexylena commented Nov 7, 2016

Sorry for delayed response, will look at this soon. Thanks for upstreaming y'all's stuff.

@hexylena
Copy link
Owner

Ok, looks good.

@hexylena hexylena merged commit d219c14 into hexylena:master Nov 15, 2016
@hexylena
Copy link
Owner

By "looks good", I mean, I'm willing to merge this to work with y'all:

  • I'm not as active in this project as I could be and I have no interest in impeding process, hence the merge.
  • However, no recent commits != dead project / galaxy parts aren't being used. The galaxy parts are definitely being used and should be treated at the same level as the cwl stuff.

There are some issues I'm correcting post-merge or will need you to correct:

  • This project has historically supported 2.7, but the tests that were added only run on 3.3+, due to the use of Mock.
    • 90% of the uses could have been avoided by using the built-in feature to pass an argv to argparse
    • The others could stand to just patch objects manually rather than using the newer library. I took a stab at fixing that and got a lot of errors from cwl side, so I'm not looking at it further. If you are still working on this, it would be good if you fixed this and added nosetests (or similar) to the .travis.yml
  • The readme was changed significantly to highlight CWL rather than a single run through saying "here's how to do this in galaxy XML, and here's the corresponding CWL commands". I have reverted this in the README, and highlight both tooling formats simultaneously.
  • Things like self.parse_args_galaxy_nouse vs self.parse_args_cwl make it seem like galaxy is a second class citizen when it's the sole reason for this project existing. This is easily corrected, but it gives me a negative impression of the author's attitude to this project.

The CWL are really quite separate from galaxy stuff, and that's a bit unfortunate, but oh well. Maybe someday we can get more generic code paths.

I am renaming this project argparse2tool, pursuant to common-workflow-lab#1

@mr-c
Copy link
Collaborator

mr-c commented Nov 15, 2016

Hello @erasche

Thank you very much for the use of your codebase and for the merge!

My apologies for not fully preparing our branch before requesting a merge back in; I should have done a thorough review.

I've updated our documentation to point to the new name; thank you for changing that.

We hope to eventually have a generic CWL serialization library and move away from the jinja templates -- hopefully that will enable cleaner and more shareable code paths.

Again, my apologies for the awkward merge.

@hexylena
Copy link
Owner

No worries Michael. Again, happy to accomodate y'all however I can. I'm
glad to see cwl support in here. This will significantly expand the utility.

Den tir. 15. nov. 2016, 08.03 skrev Michael R. Crusoe <
notifications@github.com>:

Hello @erasche https://github.com/erasche

Thank you very much for the use of your codebase and for the merge!

My apologies for not fully preparing our branch before requesting a merge
back in; I should have done a thorough review.

I've updated our documentation to point to the new name; thank you for
changing that.

We hope to eventually have a generic CWL serialization library and move
away from the jinja templates -- hopefully that will enable cleaner and
more shareable code paths.

Again, my apologies for the awkward merge.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#32 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb_u1sG821ygClaH3sFT7PsBqxPtKftks5q-Wc5gaJpZM4KnY0O
.

@hexylena
Copy link
Owner

And my apologies for grouchiness. Unrelated events sometimes get rolled up
into too emotional replies. I usually do my best to divorce emotions from
my code, but sometimes things come off overly negative.

Den tir. 15. nov. 2016, 08.04 skrev Eric Rasche rasche.eric@gmail.com:

No worries Michael. Again, happy to accomodate y'all however I can. I'm
glad to see cwl support in here. This will significantly expand the utility.

Den tir. 15. nov. 2016, 08.03 skrev Michael R. Crusoe <
notifications@github.com>:

Hello @erasche https://github.com/erasche

Thank you very much for the use of your codebase and for the merge!

My apologies for not fully preparing our branch before requesting a merge
back in; I should have done a thorough review.

I've updated our documentation to point to the new name; thank you for
changing that.

We hope to eventually have a generic CWL serialization library and move
away from the jinja templates -- hopefully that will enable cleaner and
more shareable code paths.

Again, my apologies for the awkward merge.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#32 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb_u1sG821ygClaH3sFT7PsBqxPtKftks5q-Wc5gaJpZM4KnY0O
.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants