Skip to content

feat: Seed commands tests, rework projects internals#189

Merged
yangchoo merged 8 commits intomasterfrom
SEA-719_example-commands-test
Feb 4, 2021
Merged

feat: Seed commands tests, rework projects internals#189
yangchoo merged 8 commits intomasterfrom
SEA-719_example-commands-test

Conversation

@yangchoo
Copy link
Copy Markdown
Contributor

The focus of this diff is to seed example tests using the responses
library for the commands.py file, which currently has no test coverage.

The projects and submit command, which are among the more utilized
functions are used here as examples.

In the process of adding tests, the projects internals in commands.py
was found to be confusing and made to be more intuitive.
A deprecation warning is attached to the -i option in the projects
command. Instead of requiring an input which is unused, this turns out
into a flag and renamed to --names.

@yangchoo yangchoo force-pushed the SEA-719_example-commands-test branch 3 times, most recently from cacd044 to 8aa1211 Compare January 19, 2021 10:32
The focus of this diff is to seed example tests using the responses
library for the commands.py file, which currently has no test coverage.
The `projects` and `submit` command, which are among the more utilized
functions are used here as examples.

In the process of adding tests, the projects internals in commands.py
was found to be confusing and made to be more intuitive.
A deprecation warning is attached to the `-i` option in the `projects`
command. Instead of requiring an input which is unused, this turns out
into a flag and renamed to `--names`.
@yangchoo yangchoo force-pushed the SEA-719_example-commands-test branch from 8aa1211 to f77bd5e Compare January 19, 2021 11:26
Copy link
Copy Markdown
Contributor

@ramu-phanimukla ramu-phanimukla left a comment

Choose a reason for hiding this comment

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

Looks good with few nits and questions/clarifications.

Comment thread test/commands_test.py Outdated
Comment thread test/commands_test.py Outdated
Comment thread test/commands_test.py Outdated
Comment thread test/commands_test.py Outdated
Comment thread transcriptic/commands.py
Comment thread transcriptic/commands.py
@yangchoo
Copy link
Copy Markdown
Contributor Author

@ramu-phanimukla I reworked this slightly to separate out the responsibilities of commands.py and cli.py. Making the distinction clearer that cli.py should handle the outputting to terminal and any formatting if necessary.

Let me know what you think, I can roll it out to the submit function as well if that makes sense.

Copy link
Copy Markdown
Contributor

@ramu-phanimukla ramu-phanimukla left a comment

Choose a reason for hiding this comment

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

LGTM

@yangchoo
Copy link
Copy Markdown
Contributor Author

yangchoo commented Feb 2, 2021

@ramu-phanimukla CI wasn't passing due to an unfixed dependency causing a regression. I've fixed it and it passes now.

Other than that, no other changes, would appreciate a re-review. thanks!

Copy link
Copy Markdown
Contributor

@ramu-phanimukla ramu-phanimukla left a comment

Choose a reason for hiding this comment

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

All good

@yangchoo yangchoo merged commit 5d2e135 into master Feb 4, 2021
@yangchoo yangchoo deleted the SEA-719_example-commands-test branch February 4, 2021 23:01
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