Use common code between draft and not for combining top line and rendered #303

altendky · 2020-12-15T03:53:01Z

Fixes #105.

…ered

codecov-io · 2020-12-15T04:02:08Z

Codecov Report

Merging #303 (bf67a6b) into master (19c0957) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
+ Coverage   97.04%   97.08%   +0.03%     
==========================================
  Files          22       22              
  Lines        1321     1339      +18     
  Branches      126      128       +2     
==========================================
+ Hits         1282     1300      +18     
  Misses         20       20              
  Partials       19       19

Impacted Files	Coverage Δ
src/towncrier/_builder.py	`94.28% <ø> (ø)`
src/towncrier/test/test_format.py	`100.00% <ø> (ø)`
src/towncrier/test/test_write.py	`100.00% <ø> (ø)`
src/towncrier/build.py	`96.47% <100.00%> (+0.52%)`	⬆️
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 19c0957...bf67a6b. Read the comment docs.

fschulze · 2020-12-15T07:51:49Z

I can confirm this fixes the issue.

adiroiban

Looks good.

I would say that top_line should be moved outside of `append_to_newsfile.

Also, what is single_file in the context of append_to_newsfile ? :)
If single_file is False it will make existing_content a list and compare it with top_line
Doesn't look ok :)

My suggestion is to extract the duplicate version detection code outside of append_to_newsfile. Have a dedicated name for this method and add all the checks there :)

adiroiban · 2020-12-20T12:53:07Z

src/towncrier/test/test_build.py

+                [
+                    "--version",
+                    "7.8.9",
+                    "--name",
+                    "foo",
+                    "--date",
+                    "01-01-2001",
+                    "--draft",
+                ],


Just a bit easier to read :)

Suggested change

[

"--version",

"7.8.9",

"--name",

"foo",

"--date",

"01-01-2001",

"--draft",

],

[

"--version=7.8.9",

"--name=foo",

"--date=21-01-2001",

"--draft",

],

altendky · 2020-12-20T16:22:28Z

I would say that top_line should be moved outside of `append_to_newsfile.

Perhaps I am misreading this, but isn't that what I did here? Are you confirming that is good, or suggesting something else should happen?

Also, what is single_file in the context of append_to_newsfile ? :)
If single_file is False it will make existing_content a list and compare it with top_line
Doesn't look ok :)

I think single file is the standard usage where towncrier tries to inject new news into the right place in an existing file (generally) where as not-single-file is if you make a separate output file each time you process news fragments. Maybe this is more common in bigger projects where you might have a large output file for each release and then if you want a comprehensive all-history output you could join each individual release file together. Maybe? So, if it is not a single file, you don't need to do all the reading of the existing content and injecting of new content, just write the new content over anything that may or may not be there.

I'm not super excited about this code design, but I think it works? Well, other than the fix over at #301 (comment) which you made me rethink the fix for (and I already rethought again...).

My suggestion is to extract the duplicate version detection code outside of append_to_newsfile. Have a dedicated name for this method and add all the checks there :)

What version detection code is there in append_to_newsfile()? I feel like I may have missed something here.

adiroiban · 2020-12-20T17:51:59Z

Perhaps I am misreading this, but isn't that what I did here? Are you confirming that is good, or suggesting something else sh

I was suggesting this removal :)

- def append_to_newsfile( directory, filename, start_line, top_line, content, single_file=True
+ def append_to_newsfile( directory, filename, start_line, content, single_file=True

adiroiban · 2020-12-20T17:53:52Z

What version detection code is there in append_to_newsfile()? I feel like I may have missed something here.

This is the code in which append_to_newsfile is not only appending, but also doing a sanity check to make sure we don't have duplicated notes

    if single_file:
        if not os.path.exists(news_file):
            existing_content = u""
        else:
            with io.open(news_file, "r", encoding="utf8") as f:
                existing_content = f.read()
        existing_content = existing_content.split(start_line, 1)
    else:
        existing_content = [u""]

    if top_line and top_line in existing_content:
        raise ValueError("It seems you've already produced newsfiles for this version?")

altendky · 2020-12-21T23:02:36Z

The catchup merge brought in some problems. I reworked it a bit. I didn't get top_line completely out of append_to_newsfile(), but I did remove one of the uses. Hopefully this structure for handling the top line/title completely outside the template in the default case is a bit cleaner.

Sorry, but I feel like this is enough of a change to deserve a re-review. I'm also not convinced this is a well focused PR, so 'go break this down into more pieces' is a perfectly reasonable response.

adiroiban

Thanks for the update.

Just a comment for now... but I am kind of -1 or the PR as it is now.

I am confused by the tests :)

adiroiban · 2020-12-28T10:01:34Z

src/towncrier/test/test_build.py

+                    ==============================================
+                    {% for section in sections %}
+                    {% set underline = "-" %}
+                    {% for category, val in definitions.items() if category in sections[section] %}


it looks like all these jinja directives from the template are not used in this test.

I don't understand why you would like to have title_format = false and a custom template.
With a custom template you can just don't include the title part without having to use False.

Yes, this template should be able to have all these normal template bits removed. Change incoming.

Does it matter why you would want that scenario if it is documented and released functionality? Or is this question more about making sure we have a common understanding of the feature than questioning if the situation should be tested? I haven't really thought through what I think the configuration design ought to be as I'm targeting a bugfix+feature release without introducing breaking changes (nor deprecations).

None the less, false is exactly for the case where you specify a template that provides its own title. If you don't specify false then you will (should? i think?... :[) get the default title before whatever your template does. I do feel funny about the existing non-empty string or empty string/unspecified or false possibilities for title_format. I guess I see the use cases along the lines of 'default title format and template' (unspecified or empty string), 'the default template is fine but i want to change just the title format' (non-empty string), and 'gosh darn it let me just write everything in my custom template' (false).

Hopefully this helps... this is definitely an area that strikes me as simply confusing.

Does it matter why you would want that scenario if it is documented and released functionality? Or is this question more about making sure we have a common understanding of the feature than questioning if the situation should be tested?

It's more about understanding the feature and its valid use cases.

I imagine someone reading this test 1 year in the future, looking at a bug and will say: Why you need this feature with a custom template.

So I prefer to have a test in which the feature is tested like in real file, and not using a combination of configuration option that are possible, but will never be used.

At the same time, I think that this test is wrong. We should have a full test for the default template itself.

This should also be a test for the {% if render_title %} part of the default template

adiroiban · 2020-12-28T10:05:26Z

src/towncrier/test/test_build.py

+
+    def test_with_topline_and_template_and_draft(self):
+        """
+        Spacing is proper when drafting with a topline and a template.


I am not sure what the difference between test_title_format_custom and this test.

It at least differs in that this specifies a custom template while test_title_format_custom leaves the default template in use. When writing this test it ended up a bit confusing to me too how all the bits were interacting but it did seem to recreate the issue reported at #105.

I feel like the existing implementation code had evolved and needed cleanup but as it was it offered corners where specifying a title format _with_ a custom template _and_ draft (and maybe something about the title-less feature section?) could result in some unique bug.

Understood. But I think that this test can be removed.
Now that we use the exact same output for draft and final,#105 should be fixed.

I would prefer not to have this tests as it's long and confusing.

The test only checks the output of draft.

A good test for #105 would had generated the output with and without draft and compare them to make sure they are exactly the same.

But I don't think that we need extra test for that.

I made sufficient changes that a re-review is needed.

altendky

Let me know if the explanation of the tests helps, or maybe it just reinforces the -1. I could understand that. The issue reported in #105, as I recall, was a weird corner case and perhaps I should approach it with a differently formed PR.

altendky · 2020-12-28T18:16:01Z

src/towncrier/test/test_build.py

+
+    def test_with_topline_and_template_and_draft(self):
+        """
+        Spacing is proper when drafting with a topline and a template.


It at least differs in that this specifies a custom template while test_title_format_custom leaves the default template in use. When writing this test it ended up a bit confusing to me too how all the bits were interacting but it did seem to recreate the issue reported at #105.

I feel like the existing implementation code had evolved and needed cleanup but as it was it offered corners where specifying a title format _with_ a custom template _and_ draft (and maybe something about the title-less feature section?) could result in some unique bug.

altendky · 2020-12-28T18:32:51Z

src/towncrier/test/test_build.py

+                    ==============================================
+                    {% for section in sections %}
+                    {% set underline = "-" %}
+                    {% for category, val in definitions.items() if category in sections[section] %}


Yes, this template should be able to have all these normal template bits removed. Change incoming.

Does it matter why you would want that scenario if it is documented and released functionality? Or is this question more about making sure we have a common understanding of the feature than questioning if the situation should be tested? I haven't really thought through what I think the configuration design ought to be as I'm targeting a bugfix+feature release without introducing breaking changes (nor deprecations).

None the less, false is exactly for the case where you specify a template that provides its own title. If you don't specify false then you will (should? i think?... :[) get the default title before whatever your template does. I do feel funny about the existing non-empty string or empty string/unspecified or false possibilities for title_format. I guess I see the use cases along the lines of 'default title format and template' (unspecified or empty string), 'the default template is fine but i want to change just the title format' (non-empty string), and 'gosh darn it let me just write everything in my custom template' (false).

Hopefully this helps... this is definitely an area that strikes me as simply confusing.

adiroiban

Sorry for the late reply.

My changes for this :

test_title_format_false should use the default template, to cover the logic from default template.

adiroiban · 2021-01-16T09:45:57Z

src/towncrier/test/test_build.py

+                    ==============================================
+                    {% for section in sections %}
+                    {% set underline = "-" %}
+                    {% for category, val in definitions.items() if category in sections[section] %}


Does it matter why you would want that scenario if it is documented and released functionality? Or is this question more about making sure we have a common understanding of the feature than questioning if the situation should be tested?

It's more about understanding the feature and its valid use cases.

I imagine someone reading this test 1 year in the future, looking at a bug and will say: Why you need this feature with a custom template.

So I prefer to have a test in which the feature is tested like in real file, and not using a combination of configuration option that are possible, but will never be used.

At the same time, I think that this test is wrong. We should have a full test for the default template itself.

This should also be a test for the {% if render_title %} part of the default template

adiroiban · 2021-01-16T09:52:42Z

src/towncrier/test/test_build.py

+
+    def test_with_topline_and_template_and_draft(self):
+        """
+        Spacing is proper when drafting with a topline and a template.


Understood. But I think that this test can be removed.
Now that we use the exact same output for draft and final,#105 should be fixed.

I would prefer not to have this tests as it's long and confusing.

The test only checks the output of draft.

A good test for #105 would had generated the output with and without draft and compare them to make sure they are exactly the same.

But I don't think that we need extra test for that.

src/towncrier/build.py

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

glyph · 2022-03-15T05:22:00Z

@altendky @adiroiban Is this PR still alive? I have lost the plot on the controversy over this change, and I'm not sure how to review it taking that history into account.

adiroiban

Having this blocked was my fault for commenting to much in terms of automated tests.
The tests are very verbose and might break with future changes that are not related to the draft functionality.

And there was no other towncrier developer to step in and approve it.

Since we have limited resources for towncrier development, let's merge this as the issue if fixed.

We can worry about code cleanup and reducing the test side-effects later :)

adiroiban · 2022-03-15T08:06:26Z

Thanks @glyph for the follow up.
I think that if anything is no OK with trunk now, is better to fix it in a separate PR.

webknjaz · 2022-08-23T15:45:10Z

@altendky @adiroiban any chance to release this soonish? AFAIK, this fixes poor UX in https://github.com/sphinx-contrib/sphinxcontrib-towncrier that's been annoying since about v21.

altendky added 3 commits December 14, 2020 22:48

Use common code between draft and not for combining top line and rend…

a418283

…ered

Add 105.bugfix.rst

f6385e6

flake8

137204e

altendky mentioned this pull request Dec 15, 2020

Preview different from final output #105

Closed

add test docstring

a30fa9f

altendky mentioned this pull request Dec 16, 2020

Not getting expected towncrier version #304

Closed

altendky added 4 commits December 19, 2020 17:07

Merge branch 'master' into draft_and_not_more_similar

b184c4d

add missing headline underline

52d461f

Update test_build.py

9d41915

- ,

9ed955f

altendky requested a review from adiroiban December 20, 2020 00:57

adiroiban previously approved these changes Dec 20, 2020

View reviewed changes

altendky added 3 commits December 20, 2020 16:44

Merge branch 'master' into draft_and_not_more_similar

7a49dc6

lots of changes and adjustments

ab08d55

Update 105.bugfix.rst

67e6e0f

altendky requested a review from adiroiban December 21, 2020 23:04

adiroiban reviewed Dec 28, 2020

View reviewed changes

altendky commented Dec 28, 2020

View reviewed changes

adiroiban requested changes Jan 16, 2021

View reviewed changes

Merge branch 'master' into draft_and_not_more_similar

46e3710

webknjaz reviewed May 29, 2021

View reviewed changes

src/towncrier/build.py Outdated Show resolved Hide resolved

This was referenced May 29, 2021

Add a warning when Towncrier template does not have {{ top_line }} sphinx-contrib/sphinxcontrib-towncrier#48

Open

Upgrade sphinxcontrib-towncrier to 0.2.0a0 pypa/pip#9805

Merged

This was referenced May 29, 2021

Release 21.1.2 pypa/pip#10012

Merged

Towncrier adds double heading pypa/setuptools#2666

Open

kclowes mentioned this pull request Nov 12, 2021

Add towncrier ethereum/eth-tester#221

Merged

1 task

glyph and others added 2 commits March 14, 2022 22:17

listen to the linter

e39ef07

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

Merge branch 'master' into draft_and_not_more_similar

1db285b

Merge branch 'master' into draft_and_not_more_similar

bf67a6b

adiroiban approved these changes Mar 15, 2022

View reviewed changes

adiroiban merged commit 92dc7f8 into twisted:master Mar 15, 2022

ConorMacBride mentioned this pull request Aug 30, 2022

Fix for removed top_line argument in towncrier renderer OpenAstronomy/sphinx-changelog#14

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use common code between draft and not for combining top line and rendered #303

Use common code between draft and not for combining top line and rendered #303

altendky commented Dec 15, 2020

codecov-io commented Dec 15, 2020 •

edited by codecov bot

fschulze commented Dec 15, 2020

adiroiban left a comment •

edited

adiroiban Dec 20, 2020

altendky commented Dec 20, 2020

adiroiban commented Dec 20, 2020

adiroiban commented Dec 20, 2020

altendky commented Dec 21, 2020

adiroiban left a comment

adiroiban Dec 28, 2020

altendky Dec 28, 2020

adiroiban Jan 16, 2021

adiroiban Dec 28, 2020

altendky Dec 28, 2020

adiroiban Jan 16, 2021

altendky left a comment

altendky Dec 28, 2020

altendky Dec 28, 2020

adiroiban left a comment

adiroiban Jan 16, 2021

adiroiban Jan 16, 2021

glyph commented Mar 15, 2022

adiroiban left a comment

adiroiban commented Mar 15, 2022

webknjaz commented Aug 23, 2022

Use common code between draft and not for combining top line and rendered #303

Use common code between draft and not for combining top line and rendered #303

Conversation

altendky commented Dec 15, 2020

codecov-io commented Dec 15, 2020 • edited by codecov bot

Codecov Report

fschulze commented Dec 15, 2020

adiroiban left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altendky commented Dec 20, 2020

adiroiban commented Dec 20, 2020

adiroiban commented Dec 20, 2020

altendky commented Dec 21, 2020

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altendky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph commented Mar 15, 2022

adiroiban left a comment

Choose a reason for hiding this comment

adiroiban commented Mar 15, 2022

webknjaz commented Aug 23, 2022

codecov-io commented Dec 15, 2020 •

edited by codecov bot

adiroiban left a comment •

edited