Fix incorrect handling of orphan fragment names consists of only digits #588
Conversation
|
Looking closer the true source of the error is because |
SmileyChris
force-pushed
the
alphabetic-orphan-hash
branch
from
97f3dcd to
d9b4139
Compare
|
There, that's tidier! |
SmileyChris
changed the title
| """Orphaned snippets can consist of only digits.""" | ||
| self.assertEqual( | ||
| parse_newfragment_basename("+123.feature", ["feature"]), | ||
| ("+123", "feature", 0), |
There was a problem hiding this comment.
Could you add another non-orphan test with an identifier looking like a commit sha? In the templates I've started using recently, I try to output PRs/issues, commits and arbitrary identifiers separately: https://github.com/cherrypy/cheroot/blob/3591a1c/docs/changelog-fragments.d/.towncrier-template.rst.j2#L59-L63.
It'd be nice if that remained functional. Hence an explicit test...
There was a problem hiding this comment.
I'd say that should be a separate PR -- that's an interesting functional test but not really tied to the changes being made here.
There was a problem hiding this comment.
The reason I brought it up is that I wasn't sure if this PR influences it. So it's kinda related as a possible regression. But I understand that it may be non-atomic and separate otherwise.
There was a problem hiding this comment.
Thanks for your feedback.
This test is named test_orphan_all_digits . Based on that, I think that +123 is ok.
I also think that using a git commit ref is also a valid test... but it should be a separate test.
|
It looks good. I have enabled auto-merge. Thanks for your help. I am happy to review and approve a PR that add more tests... but that should not be a blocker for this PR. |
Fixes #584
Checklist
src/towncrier/newsfragments/. Describe yourchange and include important information. Your change will be included in the public release notes.