Correct to start_string consistently #301
Conversation
Codecov Report
@@ Coverage Diff @@
## master #301 +/- ##
==========================================
+ Coverage 96.15% 96.25% +0.10%
==========================================
Files 20 20
Lines 1117 1148 +31
Branches 104 105 +1
==========================================
+ Hits 1074 1105 +31
Misses 24 24
Partials 19 19
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Looks good.
Only minor comments.
If we are doing the rename, my suggestion is to rename it to "start_marker" as I think it's a better name.
The start_string name tells us that it's the start of something and that the type is string.
I don't think that we need to include the type name in the configuration value name.
src/towncrier/test/test_build.py
Outdated
|
|
||
| def test_start_string(self): | ||
| """ | ||
| Rendered content added after specified start string. |
There was a problem hiding this comment.
| Rendered content added after specified start string. | |
| The `start_string` configuration is used to detect the starting point for inserting the generated release notes. | |
| A newline is automatically added to the configured value. |
src/towncrier/test/test_build.py
Outdated
| with open("pyproject.toml", "w") as f: | ||
| f.write(dedent("""\ | ||
| [tool.towncrier] | ||
| start_string="a different start line" |
There was a problem hiding this comment.
| start_string="a different start line" | |
| start_string="Release notes start marker" |
src/towncrier/test/test_write.py
Outdated
|
|
||
| def test_multiple_file_no_start_string(self): | ||
| """ | ||
| Multiple file output with no start string works. |
There was a problem hiding this comment.
Maybe move this to test_build to have better coverage / end to end testing.
| Multiple file output with no start string works. | |
| When no `start_string` is defined, the generated content is added at the start of the file. |
| with open("newsfragments/123.feature", "w") as f: | ||
| f.write("Adds levitation") |
There was a problem hiding this comment.
I see this pattern many times
I think it can be extracted into a helper ... and the helper can also auto call os.mkdir if not already exists.
| with open("newsfragments/123.feature", "w") as f: | |
| f.write("Adds levitation") | |
| self.createFragment('123.feature', "Adds levitation") |
README.rst
Outdated
| @@ -137,7 +137,7 @@ Towncrier has the following global options, which can be specified in the toml f | |||
| version = "1.2.3" # project version if maintained separately | |||
| name = "arbitrary project name" | |||
| template = "path/to/template.rst" | |||
| start_line = "start of generated content" | |||
| start_string = "start of generated content" | |||
There was a problem hiding this comment.
Maybe a better name and documentation
| start_string = "start of generated content" | |
| start_marker = "Text used to detect where to add the generated content in the middle of a file. Generated content added after this text. Newline auto added." |
The actual name read out of the configuration file is not being changed in this PR. The docs are being corrected and the code is being changed to match. I agree with the sentiment that the name isn't great but wasn't shooting for breaking changes here. If you agree with this path I'll skip those comments and address the rest. |
| if existing_content: | ||
| if existing_content[0]: | ||
| f.write(b"\n\n") | ||
| f.write(existing_content[0].lstrip().encode("utf8")) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -24,7 +24,7 @@ def append_to_newsfile( | |||
| else: | |||
| with io.open(news_file, "r", encoding="utf8") as f: | |||
| existing_content = f.read() | |||
| existing_content = existing_content.split(start_line, 1) | |||
| existing_content = existing_content.split(start_string, 1) | |||
| else: | |||
| existing_content = [u""] | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Yes. This can be merged with the current name (via the approved action) :) Thanks! |
Fixes #277.