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

Refactor.lint #5718

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Refactor.lint #5718

merged 2 commits into from
Nov 23, 2023

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Sep 5, 2023

I did a review of my older code as a training exercise, and started refactoring.

  • The big win - remove duplication by merging get_reference_text and get_reference_rst.
  • Turned on the Hyperlinks in the documentation. Test this by building Cylc Doc.
  • Spun as much as possible out of the main function.
  • Added typing to more functions.

There's plenty more refactoring I'd like to do, but I really shouldn't be prioritizing this.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes - I have deliberately created a chain of changes in individual commits - feel free to squash-merge.
  • 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 minimally changed.
  • 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.

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.

cylc-doc build error:

Warning, treated as error:
~/github/cylc-flow/cylc/flow/scripts/lint.py:docstring of cylc.flow.scripts.lint:10:Title underline too short.

`S001 <https://cylc.github.io/cylc-doc/stable/html/workflow-design-guide/style-guide.html#tab-characters>`_
^^^^
make: *** [html] Error 2

@wxtim wxtim marked this pull request as draft November 20, 2023 11:36
@wxtim wxtim force-pushed the refactor.lint branch 2 times, most recently from cf5159a to 641ec46 Compare November 20, 2023 12:36
@wxtim wxtim requested a review from MetRonnie November 20, 2023 12:37
@wxtim wxtim marked this pull request as ready for review November 20, 2023 12:37
Refactored get_reference_rst and get_reference_text into a single method.

Actually use the URL in rst reference mode

Added urls to CLI linter listing

move extraneous stuff away from main func

added type hinting and removed toml merging routines from the top level functions

Move the logic for checking if the target is a Cylc 8 workflow
to its own function.

We want the target to be the top level folder, not the flow.cylc file
@wxtim wxtim requested a review from MetRonnie November 22, 2023 09:52
Comment on lines -1274 to +1285
__doc__ += TOMLDOC.format(
'\n\n.. code-block:: toml\n', str(LINT_SECTIONS)) + get_reference_rst(
parse_checks(['728', 'style'], reference=True))
__doc__ += get_reference('all', 'rst')
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be

__doc__ += TOMLDOC.format(
    '\n\n.. code-block:: toml\n', str(LINT_SECTIONS)
) + get_reference('all', 'rst')

?

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think not, this was what is currently causing cylc-doc build failures but your changes fix it

Copy link
Member

Choose a reason for hiding this comment

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

(Will need a fix backported to 8.2.x)

@markgrahamdawson markgrahamdawson merged commit afa988f into cylc:master Nov 23, 2023
24 of 27 checks passed
wxtim added a commit to wxtim/cylc that referenced this pull request Dec 5, 2023
…at_runtime

* upstream/master:
  log_vc_info: handle long command output (cylc#5821)
  GH Actions: limit tutorial workflow to Py3.11
  remove cylc task dependencies env var (cylc#5836)
  Refactor.lint (cylc#5718)
  protobuf 4.24.4 upgrade (cylc#5828)
  made reinstall work on multiple workflows
  Fix `IndepQueueManager` test (cylc#5832)
  Lint: Add a check for indentation being 4N spaces. (cylc#5772)
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.

3 participants