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

only argparse #27

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

only argparse #27

wants to merge 47 commits into from

Conversation

soulios
Copy link
Contributor

@soulios soulios commented Mar 6, 2024

Changes in the utils.py for parsing all args correctly with cli overriding json.
Added options in the options.py
and remoced redundant code from main.py

Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

One more request from my side. In the Galaxy wrappers we can distinguish a label and help text. My idea would be to use the first sentence of the argument help as label and the rest as help.

I made some suggestions (you can just use them in a commit .. can show you how if needed). Could you do this for the other classes or should be do it together?

dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
dfpl/options.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Finally found some time to work on this. Sorry for the delay. With the renaming of the variables the automatic conversion of the argument parser to a Galaxy tool finally works.

Will continue now to add some tests and try to get it working (i.e. functional Galaxy tests).

For now I have one comment/question.

type=str,
choices=["fp", "smiles"],
help="Type of the chemical representation. Choices: 'fp', 'smiles'.",
default=argparse.SUPPRESS,
default="fp",
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the definition of the defaults is redundant. They are defined in the data classes and here. Would it be possible to pass an instance of the dataclasses to these functions and reuse the defaults?

For instance, for type you would need Options.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I'm not sure if this interferes with the Galaxy tool generator, so it's more a question... Maybe we can test such a change for a single parameter first.

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.

2 participants