Skip to content

Fix #321#322

Merged
jmaupetit merged 5 commits intojazzband:masterfrom
hiiwave:fix-321
Oct 1, 2019
Merged

Fix #321#322
jmaupetit merged 5 commits intojazzband:masterfrom
hiiwave:fix-321

Conversation

@hiiwave
Copy link
Copy Markdown
Contributor

@hiiwave hiiwave commented Sep 21, 2019

This is to fix #321

@hiiwave
Copy link
Copy Markdown
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
Copy Markdown
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 #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
Copy Markdown
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
Copy Markdown
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.

Comment thread CHANGELOG.md Outdated
@jmkerr
Copy link
Copy Markdown
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
Copy Markdown
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! 👍

Comment thread CHANGELOG.md Outdated
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