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

Remove c char from prereq API dump #6305

Merged
merged 1 commit into from
Aug 24, 2024
Merged

Remove c char from prereq API dump #6305

merged 1 commit into from
Aug 24, 2024

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Aug 15, 2024 •
edited
Loading

cylc/cylc-ui#1886 (comment)

Not sure why "c" was ever included in the prerequisite alias. It is somewhat confusing when you have single-letter task names as we do in test workflows.

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).
  • No dependency changes
  • Tests are updated.
  • No changelog or docs needed as not user-facing
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added this to the 8.3.4 milestone Aug 15, 2024
@MetRonnie MetRonnie self-assigned this Aug 15, 2024
@oliver-sanders
Copy link
Member

oliver-sanders commented Aug 15, 2024 •
edited
Loading

So long as this doesn't break anything, we're good to remove it.

Both cylc show and the Info view will continue to strip this character for a period as they may be communicating with workflows running under Cylc versions.

oliver-sanders
oliver-sanders reviewed Aug 15, 2024
View reviewed changes
cylc/flow/scripts/show.py Outdated
Comment on lines 310 to 315
item['expression'].replace('c', ''),
item['expression'],
item['satisfied']
])
for cond in item['conditions']:
if multi_cond and not options.list_prereqs:
prefix = f'\t{cond["exprAlias"].strip("c")} = '
Copy link
Member

@oliver-sanders oliver-sanders Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Gotta leave this in for now I'm afraid, otherwise the character will appear in show output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, forgot that cylc show needs the workflow in question to be running (which might be on an older version)

dwsutherland
dwsutherland approved these changes Aug 24, 2024
View reviewed changes
Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

LGTM

@dwsutherland
Copy link
Member

Not sure why "c" was ever included in the prerequisite alias. It is somewhat confusing when you have single-letter task names as we do in test workflows.

It was something that was present in the cylc7 api data/dump.. But really was just a shorthand for condition

@dwsutherland dwsutherland merged commit 347921f into cylc:8.3.x Aug 24, 2024
27 checks passed
@MetRonnie MetRonnie deleted the prereq-dump branch August 27, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwsutherland dwsutherland dwsutherland approved these changes

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

Assignees

@MetRonnie MetRonnie

Labels
small
Projects
None yet
Milestone
8.3.4
Development

Successfully merging this pull request may close these issues.
3 participants
@MetRonnie @oliver-sanders @dwsutherland