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

Cylc 7 ref graphs without pygtk #3349

Merged
merged 7 commits into from
Sep 5, 2019
Merged

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Sep 4, 2019

These changes close #3343

Provides a new command, cylc ref-graph SUITE to generate text-format "reference graphs" without pygtk (back-port of @oliver-sanders' cylc graph on master for Cylc 8).

Under the circumstances, I don't think there is any need to properly incorporate this into the main Cylc 7 cylc graph command.

Also updated the "misleading" PyGTK installation documentation.

Requirements 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).
  • Appropriate tests added.
  • Appropriate change log entry included.
  • (7.8.x branch) I have updated the documentation in this PR branch.

@hjoliver hjoliver added this to the cylc-7.8.5 milestone Sep 4, 2019
@hjoliver hjoliver self-assigned this Sep 4, 2019
@hjoliver
Copy link
Member Author

hjoliver commented Sep 4, 2019

(Easy review - this is just a copy of bin/cylc-graph from master, with imports modified for the old lib/cylc and small changes for the old-style command-line handling).

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.

Rather lovely code if I might say so myself.

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

I'll review this more fully shortly, but just one thought first: it could be likely users requiring this would want to do something like cylc graph-diff, in which case could we just advise them (as the situation arises, or perhaps by detail in the docstring to be seen via --help) to do a manual diff command on outputs from this command, or is it worth adding a simple interface to cylc graph-diff so it can process the outputs from cylc ref-graph as well as cylc graph --reference? Or do you think it is unlikely given it is perhaps niche not to have GTK?

bin/cylc-ref-graph Outdated Show resolved Hide resolved
Co-Authored-By: Sadie L. Bartholomew <30274190+sadielbartholomew@users.noreply.github.com>
@hjoliver
Copy link
Member Author

hjoliver commented Sep 5, 2019

it could be likely users requiring this would want to do something like cylc graph-diff, in which case could we just advise them .... Or do you think it is unlikely given it is perhaps niche not to have GTK?

That's a good point, but I think impending arrival of Cylc 8 means we shouldn't bother unless users ask for it (given that almost everyone using Cylc 7 has PyGTK installed).

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Seems to work as it should, but some minor logic changes would be desirable before merge.

bin/cylc-ref-graph Outdated Show resolved Hide resolved
bin/cylc-ref-graph Outdated Show resolved Hide resolved
bin/cylc-ref-graph Show resolved Hide resolved
@hjoliver
Copy link
Member Author

hjoliver commented Sep 5, 2019

Note I also added a test, to check that cylc graph --reference and cylc ref-graph generate identical results.

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

The new command tests as working, the new tests for it pass locally, & feedback addressed fully. Great.

@sadielbartholomew
Copy link
Collaborator

Are you happy for this to be merged @hjoliver, or do you think the latest commits warrant a re-review by Oliver?

@hjoliver
Copy link
Member Author

hjoliver commented Sep 5, 2019

I'll merge it (if this was destined for master I might wait for re-review).

@hjoliver hjoliver merged commit de7d938 into cylc:7.8.x Sep 5, 2019
@hjoliver hjoliver deleted the ref-graph-no-gtk branch September 5, 2019 23:11
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