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

Changed CLI View Prefix #774

Merged
merged 8 commits into from
Jun 19, 2022
Merged

Changed CLI View Prefix #774

merged 8 commits into from
Jun 19, 2022

Conversation

kodyt
Copy link
Contributor

@kodyt kodyt commented Jun 10, 2022

Description

I changed the CLI view argument to establish a path to be a flag rather than a positional argument. Added a test to check this change and altered another test that had signac view view path and changed it to just signac view path.

Motivation and Context

Fixes issue #653.

Checklist:

@kodyt kodyt requested review from a team as code owners June 10, 2022 15:50
@kodyt kodyt requested review from mikemhenry, syjlee and a team and removed request for a team June 10, 2022 15:50
@kodyt kodyt marked this pull request as draft June 10, 2022 16:15
@kodyt kodyt marked this pull request as ready for review June 10, 2022 16:30
@kodyt kodyt requested a review from bdice June 10, 2022 16:30
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

This looks great! However, this is an API breaking change and should target next instead of master so that it goes into version 2.0. Can you rebase this on next and change the target branch? (I can help if you want -- it is a quick process but I want to let you try it if haven't before.)

@kodyt kodyt requested a review from a team as a code owner June 14, 2022 20:26
@kodyt kodyt changed the base branch from master to next June 14, 2022 20:28
Copy link
Member

@bdice bdice 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 to me. Before merging, please update the changelog (see the PR checklist for more info). Also the documentation should be reviewed for anything that should be changed.

@bdice bdice linked an issue Jun 14, 2022 that may be closed by this pull request
tests/test_shell.py Outdated Show resolved Hide resolved
Copy link

@syjlee syjlee 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 to me.

@bdice bdice enabled auto-merge (squash) June 19, 2022 20:35
@bdice bdice merged commit d2b28c5 into next Jun 19, 2022
@bdice bdice deleted the feature/cli-view-prefix branch June 19, 2022 20:50
@vyasr
Copy link
Contributor

vyasr commented Jun 20, 2022

@bdice (or @kodyt) can you please update the changelog to reflect this PR? Thanks!

@bdice
Copy link
Member

bdice commented Jun 20, 2022

@vyasr Done. 96483c5

bdice added a commit that referenced this pull request Aug 1, 2022
* Changed prefix to --prefix to make the argument optional. Changed the duplicate view in test. Still need to add tests

* Added shorthand notation to view parser

* Added skeleton for test_view_prefix

* Extended test_view_incomplete_path_spec to test -p and --prefix

* Fixes

* Added test for optional prefix view

* Added name to contributors

* Update tests/test_shell.py

Co-authored-by: Bradley Dice <[email protected]>
bdice added a commit that referenced this pull request Oct 7, 2022
* Changed prefix to --prefix to make the argument optional. Changed the duplicate view in test. Still need to add tests

* Added shorthand notation to view parser

* Added skeleton for test_view_prefix

* Extended test_view_incomplete_path_spec to test -p and --prefix

* Fixes

* Added test for optional prefix view

* Added name to contributors

* Update tests/test_shell.py

Co-authored-by: Bradley Dice <[email protected]>
bdice added a commit that referenced this pull request Oct 27, 2022
* Changed prefix to --prefix to make the argument optional. Changed the duplicate view in test. Still need to add tests

* Added shorthand notation to view parser

* Added skeleton for test_view_prefix

* Extended test_view_incomplete_path_spec to test -p and --prefix

* Fixes

* Added test for optional prefix view

* Added name to contributors

* Update tests/test_shell.py

Co-authored-by: Bradley Dice <[email protected]>
vyasr pushed a commit that referenced this pull request Oct 30, 2022
* Changed prefix to --prefix to make the argument optional. Changed the duplicate view in test. Still need to add tests

* Added shorthand notation to view parser

* Added skeleton for test_view_prefix

* Extended test_view_incomplete_path_spec to test -p and --prefix

* Fixes

* Added test for optional prefix view

* Added name to contributors

* Update tests/test_shell.py

Co-authored-by: Bradley Dice <[email protected]>
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.

signac view CLI should not require a prefix
4 participants