-
Notifications
You must be signed in to change notification settings - Fork 93
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
config: more helpful error message on cli misuse #2974
config: more helpful error message on cli misuse #2974
Conversation
|
||
def __str__(self): | ||
return repr(self.msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will probably break the test battery, if we are happy with this approach (removing the repr
logic) I'll go through and fix any failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It broke everything last time I tried this. Good luck! 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change is good otherwise.
|
||
def __str__(self): | ||
return repr(self.msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It broke everything last time I tried this. Good luck! 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too! Merging! The one issue Codacy complains is actually a complexity increase, due to an extra if
. Not really an issue IMO 🎉
Oh, actually looking at Travis now before merging... if it's failing due to one of those flaky tests, I'll kick it a few times. |
@oliver-sanders you got two 👍 here, and I just kicked Travis (just in case) but looks like it was as you suggested, and the test battery is now broken. If you could fix it, then I think this will be ready to be merged. |
501bcae
to
c21bf37
Compare
Now conflicted! |
Have changed the base to 7.8.x, will address this on master in #2982. |
a6b8f76
to
c7b3249
Compare
c7b3249
to
6472d9b
Compare
Re-approve after re-base. @kinow please perform similar sanity check and merge. |
Looks good to me, merging! |
Response to user request for more informative feedback in the event that a user misses a
-s
flag when attempting to run a non-cycling suite e.g:It's difficult to provide suggestive feedback as it is not clear what the user's intent was. Also there are other ways we could end up with a malformed
=
separated pair on the CLI.$ cylc run mysuite mode=simulation