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

add order options to verdi process list #3002

Closed

Conversation

chrisjsewell
Copy link
Member

As it says on the tin! Particularly helpful if you want to list the last x processes, i.e. verdi process list -D desc -l 10

@coveralls
Copy link

coveralls commented Jun 13, 2019

Coverage Status

Coverage increased (+0.01%) to 72.958% when pulling 10d00c0 on chrisjsewell:verdi-proc-list-order into a27811e on aiidateam:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 72.952% when pulling 5f7c933 on chrisjsewell:verdi-proc-list-order into a27811e on aiidateam:develop.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Great options. Just a small name change request and it would be good if you could at a simple test to aiida.backends.tests.cmdline.commands.process.

@@ -280,6 +280,11 @@ def active_process_states():
type=click.Choice(['id', 'ctime']), default='ctime', show_default=True,
help='Order the entries by this attribute.')

ORDER_DIR = OverridableOption(
'-D', '--order-dir', 'order_dir',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use an abbreviation here, i.e. let's use --order-direction, especially since the flags are tab completed

@@ -280,6 +280,11 @@ def active_process_states():
type=click.Choice(['id', 'ctime']), default='ctime', show_default=True,
help='Order the entries by this attribute.')

ORDER_DIR = OverridableOption(
Copy link
Contributor

Choose a reason for hiding this comment

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

Also name this ORDER_DIRECTION

@sphuber
Copy link
Contributor

sphuber commented Jun 13, 2019

Also a question: why do your PR branches always have a commit with message CJS fix, then a shit-ton of Merge remote-tracking branch 'upstream/develop' into develop and then the actual commit with the fix?

@chrisjsewell
Copy link
Member Author

Also a question: why do your PR branches always have a commit with message CJS fix, then a shit-ton of Merge remote-tracking branch 'upstream/develop' into develop and then the actual commit with the fix?

Yeh its because of that that stupid CJS fix commit I made to my develop fork branch. So every time I've updated it, by merging in upstream/devlop, it was adding these extra commits.
I've just run git reset --hard upstream/develop, which looks to have fixed it. However, I've royally screwed up this branch now, so I'm going to close this PR and open a new one with a fresh branch.

@sphuber
Copy link
Contributor

sphuber commented Jun 14, 2019

If your fork's develop is messed up, you should do the following:

git checkout develop
git fetch upstream
git reset --hard upstream/develop
git push -f fork develop

The key is to push to your fork again, otherwise you will only just fix it locally.
Also, the best way for PRs in terms of a workflow is to always start from the upstream/develop.
You never really need your fork's `develop.
So always something like:

git fetch upstream
git checkout -b fix_something upstream/develop
git commit # after changes of course
git push fork fix_something

And then make a PR off of that branch.
Then if you need to update, just use rebase instead of merge, i.e.

git fetch upstream
git rebase upstream/develop

@chrisjsewell chrisjsewell deleted the verdi-proc-list-order branch June 23, 2019 12:40
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.

None yet

3 participants