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

Fix cylc lint U013/U015 missing info #6214

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Fix cylc lint U013/U015 missing info #6214

merged 1 commit into from
Aug 22, 2024

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jul 10, 2024

closes #6205

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).
  • CHANGES.md 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.

changes.d/6178.fix.md Outdated Show resolved Hide resolved
@wxtim wxtim self-assigned this Jul 10, 2024
@wxtim wxtim requested a review from MetRonnie July 10, 2024 09:25
@wxtim wxtim modified the milestones: 8.3.3, 8.3.2 Jul 10, 2024
@MetRonnie MetRonnie changed the base branch from master to 8.3.x July 10, 2024 17:15
@MetRonnie MetRonnie added bug Something is wrong :( small labels Jul 10, 2024
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

There's a similar issue for

'U015': {
'short': (
'Deprecated template variables.'),
'rst': (
'The following template variables, mostly used in event handlers,'
'are deprecated, and should be replaced:'
+ ''.join([
f'\n * ``{old}`` ⇒ {new}'
for old, new in DEPRECATED_STRING_TEMPLATES.items()
])
),
'url': (
'https://cylc.github.io/cylc-doc/stable/html/user-guide/'
'writing-workflows/runtime.html#task-event-template-variables'
),
FUNCTION: check_for_deprecated_task_event_template_vars,

expired handlers = %(suite_uuid)s %(user@host)s

@wxtim wxtim marked this pull request as draft July 12, 2024 10:08
@wxtim
Copy link
Member Author

wxtim commented Jul 12, 2024

There's a similar issue for

Re-drafted - I'm going to write an automated test for this.

@wxtim wxtim marked this pull request as ready for review July 12, 2024 14:20
@wxtim wxtim requested a review from MetRonnie July 12, 2024 14:20
Copy link
Member Author

Choose a reason for hiding this comment

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

This is Separate - I have in my mind that at some point I'm going to refactor out those aspects of Cylc lint which are data rather than infrastructure.

cylc/flow/scripts/lint.py Show resolved Hide resolved
tests/unit/scripts/test_lint_checkers.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from MetRonnie July 17, 2024 11:49
@wxtim wxtim modified the milestones: 8.3.3, 8.3.4 Jul 23, 2024
@MetRonnie

This comment was marked as resolved.

@@ -673,7 +687,7 @@ def list_wrapper(line: str, check: Callable) -> Optional[Dict[str, str]]:
},
'U015': {
'short': (
'Deprecated template variables.'),
'Deprecated template variables: {suggest}'),
Copy link
Member

Choose a reason for hiding this comment

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

Rather than have to rely on variable names matching here (suggest in this case but vars elsewhere), why not just make the check functions return a list of strings, and add the joined list to the end of the short message?

Copy link
Member Author

@wxtim wxtim Aug 5, 2024

Choose a reason for hiding this comment

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

Allows us to insert multiple variables

msg = check_meta['short'].format(**check)

It's used by one check.

I've re-drafted this PR - I want to have a think about where I'm going with this....

Copy link
Member Author

Choose a reason for hiding this comment

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

Un drafted - I think that my long term goal is to have a better defined check API so that others can add them, but I think I want this fixed sooner, and will worry about that later.

@wxtim wxtim marked this pull request as draft August 5, 2024 10:57
@wxtim wxtim marked this pull request as ready for review August 16, 2024 13:58
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Some minor suggestions

changes.d/6214.fix.md Outdated Show resolved Hide resolved
tests/unit/scripts/test_lint_checkers.py Outdated Show resolved Hide resolved
tests/unit/scripts/test_lint_checkers.py Outdated Show resolved Hide resolved
tests/unit/scripts/test_lint_checkers.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from oliver-sanders August 22, 2024 07:41
Lint: Testing for check function outputting values if check['short'] needs them

Apply suggestions from code review
@MetRonnie MetRonnie merged commit 14d4ec0 into cylc:8.3.x Aug 22, 2024
27 checks passed
@MetRonnie MetRonnie changed the title fix.lint.U013 incomplete message Fix cylc lint U013/U015 missing info Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cylc lint and CYLC_SUITE_DEF_PATH variable (rule U013) message is not complete
2 participants