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

Raise ConfigError for missing newsfragment directory #298

Merged
merged 12 commits into from
Dec 19, 2020

Conversation

altendky
Copy link
Member

Fixes #85

@codecov-io
Copy link

codecov-io commented Dec 13, 2020

Codecov Report

Merging #298 (7e0be3e) into master (6023801) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
src/towncrier/_builder.py 94.28% <100.00%> (+0.43%) ⬆️
src/towncrier/test/test_build.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6023801...7e0be3e. Read the comment docs.

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

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.

src/towncrier/newsfragments/85.bugfix.rst Outdated Show resolved Hide resolved
@altendky altendky requested a review from adiroiban December 19, 2020 16:12
@altendky altendky changed the title Treat a missing newsfragment directory as if it is empty Raise ConfigError for missing newsfragment directory Dec 19, 2020
@adiroiban
Copy link
Member

My view now
image

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
Comment on lines 111 to 112
# This should fail
self.assertEqual(result.exit_code, 0)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Suggested change
# 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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the comment may have been a copy/paste error from the test after this.

@altendky
Copy link
Member Author

Kick CI to watch try to observe codecov responses a bit.

@altendky altendky closed this Dec 19, 2020
@altendky altendky reopened this Dec 19, 2020
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Merge it :) Thanks!

src/towncrier/test/test_build.py Outdated Show resolved Hide resolved
@altendky altendky merged commit 96178dc into twisted:master Dec 19, 2020
@altendky altendky deleted the no_directory_same_as_no_fragments branch December 19, 2020 19:17
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.

newsfragments directory not created
3 participants