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 #321 #322

Merged
merged 5 commits into from
Oct 1, 2019
Merged

Fix #321 #322

merged 5 commits into from
Oct 1, 2019

Conversation

hiiwave
Copy link
Contributor

@hiiwave hiiwave commented Sep 21, 2019

This is to fix #321

@hiiwave
Copy link
Contributor Author

hiiwave commented Sep 21, 2019

The tests run well under my environment. I have no idea yet why the CI failed here. (help wanted)

@hiiwave hiiwave closed this Sep 21, 2019
@hiiwave hiiwave reopened this Sep 21, 2019
Copy link
Contributor

@davidag davidag left a comment

Choose a reason for hiding this comment

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

Hello @hiiwave, let me give you my opinion on these changes:

  • I would try to avoid raising WatsonError in cli.py. Just use click.ClickException which is in fact what _watson.WatsonError is (still) in cli.py. See Unify WatsonError catching in cli.py #315
  • I'm not sure the check for empty project at the beginning of `Watson.start() is still necessary. Did you try to remove and run the tests?
  • Could you add a line in the changelog describing the fix?

I hope it helps!

@hiiwave
Copy link
Contributor Author

hiiwave commented Sep 22, 2019

Agree. Thanks for the suggestions. Changes applied.

Could you add a line in the changelog describing the fix?

Sure. Is this a common practice for PR here? As I didn't do this in #312, #318 and #320 .

@davidag
Copy link
Contributor

davidag commented Sep 22, 2019

Agree. Thanks for the suggestions. Changes applied.

@jmkerr do you have any clue of why test_report_current is failing with the changes in this PR? Thanks! ☺️

Could you add a line in the changelog describing the fix?

Sure. Is this a common practice for PR here? As I didn't do this in #312, #318 and #320 .

As far as I have seen, it is common practice here.

CHANGELOG.md Outdated Show resolved Hide resolved
@jmkerr
Copy link
Contributor

jmkerr commented Sep 26, 2019

@jmkerr do you have any clue of why test_report_current is failing with the changes in this PR? Thanks! ☺️

This is completely unrelated to the content of the commit, but possibly related to the time the test was run between 00:00 and 01:00 UTC. I'll take a look at the problem and open another issue. (I'm sorry.)

@davidag
Copy link
Contributor

davidag commented Sep 26, 2019

This is completely unrelated to the content of the commit, but possibly related to the time the test was run between 00:00 and 01:00 UTC. I'll take a look at the problem and open another issue. (I'm sorry.)

@jmkerr thank you for your help! If there's some way we can help you out, just say it! 👍

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Julien Maupetit <jmaupetit@users.noreply.github.com>
@jmaupetit jmaupetit merged commit 4dc7e23 into jazzband:master Oct 1, 2019
@jmaupetit jmaupetit mentioned this pull request May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

watson start should not accept empty project
5 participants