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

Move to entry points #3413

Merged
merged 27 commits into from
Dec 9, 2019
Merged

Conversation

sgaist
Copy link
Contributor

@sgaist sgaist commented Oct 16, 2019

These changes partially address #2802, #2972

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).
  • Already covered by existing tests.
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@hjoliver hjoliver added this to the cylc-8.0.0 milestone Oct 16, 2019
@hjoliver
Copy link
Member

A bunch of minor code style failures revealed on Travis CI.

@sgaist sgaist force-pushed the move_to_entry_points branch 3 times, most recently from 08a9664 to 4b4fff6 Compare October 16, 2019 21:34
@hjoliver
Copy link
Member

@sgaist - there's still a bunch of test failures. See /etc/bin/run-functional-tests.sh for how to run them locally (individually, or many, or all of them - not recommended). Ask if you need help understanding what's going on (some functional tests are hard to diagnose ... but these failures should be relatively easy, e.g. I can see a bunch for command help messages printed in the Travis CI logs which indicates simple command line errors).

@sgaist sgaist force-pushed the move_to_entry_points branch 2 times, most recently from e3e416c to 8d25b4b Compare October 18, 2019 21:30
@cylc cylc deleted a comment Oct 18, 2019
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Very close! Several tests passing, but found several under tests/validation failing, probably due to -v being verbose for cylc validate (and possibly other subcommands). Added comment with links/pointers to what I think is the problem.

Thank you @sgaist

cylc/flow/scripts/cylc.py Outdated Show resolved Hide resolved
cylc/flow/scripts/cylc.py Outdated Show resolved Hide resolved
@sgaist
Copy link
Contributor Author

sgaist commented Oct 22, 2019

One thing that is currently giving me some trouble: the help option.

There's currently all kinds of possible variants to get help on something. One that I havent seen before is --help something. Usually --help on -h is a flag that shows the associated help message. It is the first time I see it as an "option with argument". Therefore I am wondering whether it's something that should be more command line compliant ?

We would then have:

  • cylc -> shows help
  • cylc --h, --help, same as cylc alone
  • cylc help "something" shows the help about something

What do you guys think about that ?

@hjoliver
Copy link
Member

What do you guys think about that ?

Happy to have that standardized. You can get rid of any such weird historical anachronisms 😁

@hjoliver
Copy link
Member

But @sgaist cylc [sub-command] --help is standard, for sub-command help, isn't it? (you haven't listed that).

@dwsutherland
Copy link
Member

But @sgaist cylc [sub-command] --help is standard, for sub-command help, isn't it? (you haven't listed that).

It's how git does it.. i.e git cherry-pick --help

@sgaist
Copy link
Contributor Author

sgaist commented Oct 23, 2019

But @sgaist cylc [sub-command] --help is standard, for sub-command help, isn't it? (you haven't listed that).

I see I haven't been precise enough. I meant it only for the main entry point.

But @sgaist cylc [sub-command] --help is standard, for sub-command help, isn't it? (you haven't listed that).

It's how git does it.. i.e git cherry-pick --help

Agreed, that's why it wasn't listed and that part is already working.

@oliver-sanders
Copy link
Member

oliver-sanders commented Oct 23, 2019

Happy to have that standardized. You can get rid of any such weird historical anachronisms grin

We can definitely standardise behaviour!

@cylc cylc deleted a comment Oct 24, 2019
@cylc cylc deleted a comment Oct 27, 2019
@sgaist
Copy link
Contributor Author

sgaist commented Oct 28, 2019

One question about the tests, is there any way to run single tests or more specifically the ones that are currently failing explicitly ?

While it seems pretty thorough, which is nice, the current test suite is a bit difficult to dive in.

@cylc cylc deleted a comment Oct 28, 2019
@kinow
Copy link
Member

kinow commented Oct 28, 2019

One question about the tests, is there any way to run single tests or more specifically the ones that are currently failing explicitly ?

While it seems pretty thorough, which is nice, the current test suite is a bit difficult to dive in.

Hi @sgaist , yes, you can use ./etc/bin/run-functional-tests.sh -v $test. For example:

$ ./etc/bin/run-functional-tests.sh -v ./tests/api-suite-info/00-get-graph-raw-1.t

I use virtualenv, so before running this I activate my virtual environment to get the Cylc modules/dependencies in place first. Then I look at the output of Travis - normally I run this command to fix something I broke in a PR - copy the name of the failed test, and run with the command above.

Hope that helps 👍

@kinow
Copy link
Member

kinow commented Oct 28, 2019

ps: some tests may fail occasionally on Travis, but pass on your environment. Which is frustrating. But if they pass on your environment, it should be fine to just say that here, and one of us can try that too, or kick travis until the build passes 👍 Thanks!!!

@hjoliver
Copy link
Member

@sgaist - the functional tests can be difficult to diagnose if you don't have much Cylc background (annoying, but it's just hard to test a workflow engine). In this case any failures should be pretty straightforward, but if you find it too maddening we might be able to dive in and help.

@cylc cylc deleted a comment Nov 2, 2019
@cylc cylc deleted a comment Nov 2, 2019
@oliver-sanders
Copy link
Member

Two test failures unfortunately :(

  • tests/cylc-subscribe/01-subscribe.t (new command added in rebase)
  • tests/cli/01-help.t (command alias missing)

@hjoliver
Copy link
Member

hjoliver commented Dec 8, 2019

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 78
           

Complexity increasing per file
==============================
- cylc/flow/scripts/cylc_print.py  27
- cylc/flow/scripts/cylc_check_versions.py  21
- cylc/flow/scripts/cylc_run.py  1
- cylc/flow/scripts/cylc_get_directory.py  1
- cylc/flow/scripts/cylc_ping.py  5
- cylc/flow/scripts/cylc_dump.py  5
- cylc/flow/scripts/cylc_checkpoint.py  1
- cylc/flow/scripts/cylc_kill.py  2
- cylc/flow/scripts/cylc_hold.py  5
- cylc/flow/scripts/cylc_help.py  23
- cylc/flow/scripts/cylc_jobs_poll.py  2
- cylc/flow/scripts/cylc_get_suite_config.py  7
- cylc/flow/scripts/cylc_suite_state.py  25
- cylc/flow/scripts/cylc.py  32
- cylc/flow/scripts/cylc_cycle_point.py  36
- cylc/flow/scripts/cylc_function_run.py  3
- cylc/flow/scripts/cylc_poll.py  2
- cylc/flow/scripts/cylc_ls_checkpoints.py  5
- cylc/flow/scripts/cylc_jobs_submit.py  2
- cylc/flow/scripts/cylc_reset.py  7
- cylc/flow/scripts/cylc_set_verbosity.py  2
- cylc/flow/scripts/cylc_spawn.py  1
- cylc/flow/scripts/cylc_broadcast.py  22
- cylc/flow/scripts/cylc_report_timings.py  7
- cylc/flow/scripts/cylc_insert.py  3
- cylc/flow/scripts/cylc_get_site_config.py  4
- cylc/flow/scripts/cylc_ext_trigger.py  7
- cylc/flow/scripts/cylc_get_suite_version.py  1
- cylc/flow/scripts/cylc_remote_init.py  2
- cylc/flow/scripts/cylc_remove.py  1
- cylc/flow/scripts/cylc_client.py  3
- cylc/flow/scripts/cylc_edit.py  14
- cylc/flow/scripts/cylc_restart.py  1
- cylc/flow/scripts/cylc_get_suite_contact.py  4
- cylc/flow/scripts/cylc_register.py  1
- cylc/flow/scripts/cylc_graph.py  14
- cylc/flow/scripts/cylc_reload.py  1
- cylc/flow/scripts/cylc_check_software.py  13
- cylc/flow/scripts/cylc_show.py  32
- cylc/flow/scripts/cylc_stop.py  14
- cylc/flow/scripts/cylc_validate.py  19
- cylc/flow/scripts/cylc_nudge.py  1
- cylc/flow/scripts/cylc_submit.py  21
- cylc/flow/scripts/cylc_cat_log.py  44
- cylc/flow/scripts/cylc_search.py  20
- cylc/flow/scripts/cylc_monitor.py  83
- cylc/flow/scripts/cylc_view.py  13
- cylc/flow/scripts/cylc_diff.py  9
- cylc/flow/scripts/cylc_release.py  3
- cylc/flow/scripts/cylc_subscribe.py  10
- cylc/flow/scripts/cylc_remote_tidy.py  2
- cylc/flow/scripts/cylc_extract_resources.py  4
- cylc/flow/scripts/cylc_jobs_kill.py  2
- cylc/flow/scripts/cylc_get_host_metrics.py  8
- cylc/flow/scripts/cylc_scan.py  21
- cylc/flow/scripts/cylc_list.py  24
- cylc/flow/scripts/cylc_trigger.py  18
- cylc/flow/scripts/cylc_message.py  14
         

See the complete overview on Codacy

@hjoliver
Copy link
Member

hjoliver commented Dec 9, 2019

Kicked Travis CI as the failed test tests/triggering/08-fam-finish-any.t passes on this branch in my environment...

@oliver-sanders
Copy link
Member

oliver-sanders commented Dec 9, 2019

Kicking again!
...
Passed.

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.

Conflicts resolved, tests passing 🍾.

Thanks for your efforts @sgaist, most Cylc changes go through a lot easier, honest!

@oliver-sanders oliver-sanders modified the milestones: cylc-8.0.0, cylc-8.0a2 Dec 9, 2019
@oliver-sanders oliver-sanders merged commit 6bb7662 into cylc:master Dec 9, 2019
@sgaist sgaist deleted the move_to_entry_points branch December 9, 2019 23:07
@kinow
Copy link
Member

kinow commented Dec 9, 2019

@sgaist great work! Thanks a lot for your contribution and patience 👏 👏

@hjoliver
Copy link
Member

hjoliver commented Dec 9, 2019

@sgaist, many thanks from me too, good job 💐

@sgaist
Copy link
Contributor Author

sgaist commented Dec 9, 2019

@oliver-sanders No worries, I'm used to more difficult merges :-)

You're welcome guys !

As next "big" step, may I suggest to add pre-commit like talked in cylc-admin issue 64 ?

This would partly help cleanup the warnings that have appeared on codacy and help automate coding style standardisation. It can then be applied either once on the whole code base (but might be a bit aggressive with regards to other pull request) or file by file when one is modified.

@kinow
Copy link
Member

kinow commented Dec 9, 2019

+1 for the pre-commit hook @sgaist. Is that something you'd like to submit as a PR? Or could help testing perhaps?

@dwsutherland
Copy link
Member

@sgaist - Nice work! (and thanks sorting the conflicts I introduced!) 🍻

@hjoliver
Copy link
Member

I'm in favour of pre-commit hook. We can discuss more on the cylc-admin issue.

@sgaist
Copy link
Contributor Author

sgaist commented Dec 15, 2019

@kinow Do you mean submit a possible configuration to get started on cylc-flow ?

If so, sure thing, I can do that. I'll just wait the confirmation to avoid noise.

@kinow
Copy link
Member

kinow commented Dec 15, 2019

@sgaist yes, if you have time tgat would be great, please ☺

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.

5 participants