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

wip: Adding all configuration options as arguments #988

Merged
merged 4 commits into from
Nov 12, 2024

Conversation

ns-rse
Copy link
Collaborator

@ns-rse ns-rse commented Oct 30, 2024

  • Renames toposats.run_topostats > topostats.run_modules as its more descriptive of what the submodule is doing.
  • Moves generic command line options to be arguments of the parser.
  • Adds arguments to each sub-parser for all module options.
  • Adds tests to check that each argument is correctly returned.
  • Updated and made a few names consistent.

This is still a Work In Progress but it shouldn't break anything (tests have all passed locally).

Whether to merge this depends perhaps on the desire to tag and make a release and whether there is anything else pending that is considered essential to be in that release (e.g. documentation). If so then don't merge this.

- Renames `toposats.run_topostats` > `topostats.run_modules` as its more descriptive of what the submodule is doing.
- Moves generic command line options to be arguments of the parser.
- Adds arguments to each sub-parser for all module options.
- Adds tests to check that each argument is correctly returned.
- Updated and made a few names consistent.

This is still a Work In Progress but it shouldn't break anything (tests have all passed locally).

Whether to merge this depends perhaps on the desire to tag and make a release and whether there is anything else pending
that is considered essential to be in that release (e.g. documentation). If so then _don't_ merge this.
@ns-rse ns-rse added the enhancement New feature or request label Oct 30, 2024
@ns-rse ns-rse marked this pull request as ready for review October 30, 2024 22:58
@ns-rse
Copy link
Collaborator Author

ns-rse commented Nov 11, 2024

Merge conflict resolved and a temporary fix to get around a problem with pytest-regtest-2.3.2 under OSX. This is now ready for review again.

Copy link
Collaborator

@SylviaWhittle SylviaWhittle left a comment

Choose a reason for hiding this comment

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

No issues that I can see 😄

Thank you so much for this work 🙏

tests/test_run_modules.py Show resolved Hide resolved
topostats/default_config.yaml Show resolved Hide resolved
topostats/default_config.yaml Outdated Show resolved Hide resolved
topostats/default_config.yaml Outdated Show resolved Hide resolved
topostats/default_config.yaml Outdated Show resolved Hide resolved
@ns-rse ns-rse force-pushed the ns-rse/517-swiss-army-knife branch from dd5cad4 to 56b7777 Compare November 12, 2024 15:14
@ns-rse ns-rse merged commit 835e00f into main Nov 12, 2024
11 checks passed
@ns-rse ns-rse deleted the ns-rse/517-swiss-army-knife branch November 12, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants