Raise ConfigError for missing newsfragment directory#298
Raise ConfigError for missing newsfragment directory#298altendky merged 12 commits intotwisted:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
==========================================
+ Coverage 95.47% 95.57% +0.10%
==========================================
Files 20 20
Lines 1060 1085 +25
Branches 104 105 +1
==========================================
+ Hits 1012 1037 +25
Misses 27 27
Partials 21 21
Continue to review full report at Codecov.
|
adiroiban
left a comment
There was a problem hiding this comment.
Thanks for working on fixing this bug.
I am -1 on this.
I think that is correct to not have towcrier create directories.
But I think that the error should not be ignored.
I am running towncrier draft as part of by continuous testing.
I want to see an error. For example if I have a typo in the path.
So instead of ignoring, just raise a ConfigError with a message that the news fragment couldn't be retrieved. It might be a missing path, it might be an OS permission error.
Just show a message like: "Failed to list the news fragment files. OS_ERROR_HERE"
And people should know what is wrong from the OS ERROR message.
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
|
My view now |
adiroiban
left a comment
There was a problem hiding this comment.
For the title of PR, this PR is ok.
I would prefer to also see a non-zero exit code implemented in this PR :)
But this can be merged, assuming that we have a follow up issue for the non-zero exit code.
Thanks!
src/towncrier/test/test_build.py
Outdated
| # This should fail | ||
| self.assertEqual(result.exit_code, 0) |
There was a problem hiding this comment.
I am not sure what is going on here.
I guess that this is just a TODO due to some strange code in Click, and in the change we will have a non-zero exit code here.
| # This should fail | |
| self.assertEqual(result.exit_code, 0) | |
| # For now, exit code is zero, but it should be non-zero. | |
| # See: LINK TO ISSUE | |
| self.assertEqual(result.exit_code, 0) |
There was a problem hiding this comment.
Let's disregard my comment, I'll remove it. Do you want the pre-existing behavior of creating output with 'no significant changes' as the text and a zero exit code? Or, are we just confusing each other starting with my bad comment I left laying around.
There was a problem hiding this comment.
Looks like the comment may have been a copy/paste error from the test after this.
|
Kick CI to watch try to observe codecov responses a bit. |
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
Fixes #85