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.

Sign up for GitHub

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

Jump to bottom

Sort cylc show task proxies #6270

Merged
merged 3 commits into from
Aug 6, 2024
Merged

Sort cylc show task proxies #6270

merged 3 commits into from
Aug 6, 2024

Conversation

ColemanTom
Copy link
Contributor

@ColemanTom ColemanTom commented Jul 31, 2024 •
edited by hjoliver
Loading

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

Closes #6266

Before

Task ID: 20240728T0000Z/b
Task ID: 20240727T0000Z/b
Task ID: 20240729T0000Z/b
Task ID: 20240730T0000Z/b
Task ID: 20240731T0000Z/b
Task ID: 20240801T0000Z/b

After

Task ID: 20240727T0000Z/b
Task ID: 20240728T0000Z/b
Task ID: 20240729T0000Z/b
Task ID: 20240730T0000Z/b
Task ID: 20240731T0000Z/b
Task ID: 20240801T0000Z/b

Still works for no matches

$ cylc show basic-workflow //*/d
No matching active tasks found: */d

Notes

No tests added as I didn't think they were really needed.

@hjoliver
Copy link
Member

No tests added as I didn't think they were really needed.

It should have a test if we want to ensure that sorted output stays sorted in future.

There's an integration test that tests cylc show output for a single task, it could probably be modified to add a couple more tasks and check sort order. tests/integration/scripts/test_show.py

@hjoliver
Copy link
Member

Looks good though - thanks for the PR!

@ColemanTom
Copy link
Contributor Author

No tests added as I didn't think they were really needed.

It should have a test if we want to ensure that sorted output stays sorted in future.

There's an integration test that tests cylc show output for a single task, it could probably be modified to add a couple more tasks and check sort order. tests/integration/scripts/test_show.py

I can have a look later. Timing matters as the results depend on graphql return order, so depending on what it is doing behind the scenes, they may come back "sorted" already. I can probably rig something up which always returns an unsorted order as I did for myself fairly easily manually.

@ColemanTom ColemanTom changed the title Sort cylc show task proxies DRAFT: Sort cylc show task proxies Jul 31, 2024
@hjoliver
Copy link
Member

There are also some functional tests that literally run the script.

@hjoliver hjoliver marked this pull request as draft July 31, 2024 05:52
@hjoliver hjoliver changed the title DRAFT: Sort cylc show task proxies Sort cylc show task proxies Jul 31, 2024
@hjoliver
Copy link
Member

(I just converted the PR to a Draft, since you literally had "DRAFT" in the title - makes it clearer when listening open PRs etc.)

@MetRonnie
Copy link
Member

@ColemanTom If you would like this in the next 8.3 release, please rebase onto the 8.3.x branch

@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 31, 2024 •
edited
Loading

Otherwise leave on the master branch and it will go into 8.4.0.

Features normally go onto minor releases rather the bugfix releases, but this is so small we could go either way.

oliver-sanders
oliver-sanders approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

LGTM, I think this is ready to go (pending a milestone).

Tests wouldn't hurt, but sorting is non-functional so fair enough.

hjoliver
hjoliver approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Approving the code change too. IMO it can go to 8.3.x (we can argue it was a bug that the output order is random).

@ColemanTom
Copy link
Contributor Author

8.3 vs 8.4 - I don't particularly care. Please advise which you want it to be in and I will do that.

@hjoliver
Copy link
Member

hjoliver commented Aug 1, 2024

I assumed you'd prefer 8.3 so I was justifying that - because the next 8.3.x release will be soon, but 8.4.0 might be some time coming.

Anyhow, might as well go for 8.3.x

@hjoliver hjoliver added this to the 8.3.4 milestone Aug 1, 2024
@hjoliver
Copy link
Member

hjoliver commented Aug 1, 2024

I've changed the milestone to 8.3.4. You just need to rebase your branch locally to 8.3.x, force-push it to GitHub, then change the PR base branch to 8.3.x via Edit (PR header) above.

@ColemanTom
Copy link
Contributor Author

I've changed the milestone to 8.3.4. You just need to rebase your branch locally to 8.3.x, force-push it to GitHub, then change the PR base branch to 8.3.x via Edit (PR header) above.

I'll try to do it tomorrow or otherwise early next week. The external platform I do development on has been down for maintenance all day.

@ColemanTom
Sort taskProxies in cylc show
2a4401c 
It was possible for taskProxies when doing 'cylc show' to be in an odd order:

Task ID: 20240728T0000Z/b
Task ID: 20240727T0000Z/b
Task ID: 20240729T0000Z/b

After this change, they are always sorted by the ID of the task:

Task ID: 20240727T0000Z/b
Task ID: 20240728T0000Z/b
Task ID: 20240729T0000Z/b
@ColemanTom
Add a changelog entry for sorting 'cylc show' task proxies
fa526dd
@ColemanTom ColemanTom changed the base branch from master to 8.3.x August 2, 2024 05:12
@ColemanTom
Copy link
Contributor Author

Ok, pointing at 8.3.x now. I do want to add some tests, but I don't have the time right now but will have it in my backlog to look at when not trying to get some systems operational.

@hjoliver
Copy link
Member

hjoliver commented Aug 5, 2024

. I do want to add some tests, but I don't have the time right now but will have it in my backlog to look at when not trying to get some systems operational.

I have some tests almost ready to go for this, so you're off the hook.

@hjoliver hjoliver marked this pull request as ready for review August 6, 2024 05:44
hjoliver
hjoliver approved these changes Aug 6, 2024
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Integration test added. Just need a final approval from you @oliver-sanders

@oliver-sanders oliver-sanders merged commit 9220919 into cylc:8.3.x Aug 6, 2024
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliver-sanders oliver-sanders oliver-sanders approved these changes

@hjoliver hjoliver hjoliver approved these changes

Assignees

@hjoliver hjoliver

@ColemanTom ColemanTom

Labels
None yet
Projects
None yet
Milestone
8.3.4
Development

Successfully merging this pull request may close these issues.

Cylc show output isn't sorted by cyclepoint
4 participants
@ColemanTom @hjoliver @MetRonnie @oliver-sanders