-
Notifications
You must be signed in to change notification settings - Fork 124
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
Use common code between draft and not for combining top line and rendered #303
Changes from all commits
a418283
f6385e6
137204e
a30fa9f
b184c4d
52d461f
9d41915
9ed955f
7a49dc6
ab08d55
67e6e0f
46e3710
e39ef07
1db285b
bf67a6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
The extra newline between the title and rendered content when using ``--draft`` is no longer inserted. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -740,12 +740,25 @@ def test_title_format_false(self): | |
[tool.towncrier] | ||
package = "foo" | ||
title_format = false | ||
template = "template.rst" | ||
""")) | ||
os.mkdir("foo") | ||
os.mkdir("foo/newsfragments") | ||
# Towncrier ignores .rst extension | ||
with open("foo/newsfragments/124.feature.rst", "w") as f: | ||
f.write("Extends levitation") | ||
with open("template.rst", "w") as f: | ||
f.write(dedent("""\ | ||
Here's a hardcoded title added by the template | ||
============================================== | ||
{% for section in sections %} | ||
{% set underline = "-" %} | ||
{% for category, val in definitions.items() if category in sections[section] %} | ||
|
||
{% for text, values in sections[section][category]|dictsort(by='value') %} | ||
- {{ text }} | ||
|
||
{% endfor %} | ||
{% endfor %} | ||
{% endfor %} | ||
""")) | ||
|
||
result = runner.invoke( | ||
_main, | ||
|
@@ -767,13 +780,8 @@ def test_title_format_false(self): | |
Draft only -- nothing has been written. | ||
What is seen below is what would be written. | ||
|
||
FooBarBaz 7.8.9 (20-01-2001) | ||
============================ | ||
|
||
Features | ||
-------- | ||
|
||
- Extends levitation (#124) | ||
Here's a hardcoded title added by the template | ||
============================================== | ||
|
||
""") | ||
|
||
|
@@ -836,3 +844,65 @@ def test_start_string(self): | |
""") | ||
|
||
self.assertEqual(expected_output, output) | ||
|
||
def test_with_topline_and_template_and_draft(self): | ||
""" | ||
Spacing is proper when drafting with a topline and a template. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure what the difference between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It at least differs in that this specifies a custom template while 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood. But I think that this test can be removed. 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. |
||
""" | ||
runner = CliRunner() | ||
|
||
with runner.isolated_filesystem(): | ||
with open("pyproject.toml", "w") as f: | ||
f.write(dedent("""\ | ||
[tool.towncrier] | ||
title_format = "{version} - {project_date}" | ||
template = "template.rst" | ||
|
||
[[tool.towncrier.type]] | ||
directory = "feature" | ||
name = "" | ||
showcontent = true | ||
""")) | ||
os.mkdir("newsfragments") | ||
with open("newsfragments/123.feature", "w") as f: | ||
f.write("Adds levitation") | ||
with open("template.rst", "w") as f: | ||
f.write(dedent("""\ | ||
{% for section in sections %} | ||
{% set underline = "-" %} | ||
{% for category, val in definitions.items() if category in sections[section] %} | ||
|
||
{% for text, values in sections[section][category]|dictsort(by='value') %} | ||
- {{ text }} | ||
|
||
{% endfor %} | ||
{% endfor %} | ||
{% endfor %} | ||
""")) | ||
|
||
result = runner.invoke( | ||
_main, | ||
[ | ||
"--version=7.8.9", | ||
"--name=foo", | ||
"--date=20-01-2001", | ||
"--draft", | ||
], | ||
) | ||
|
||
expected_output = dedent("""\ | ||
Loading template... | ||
Finding news fragments... | ||
Rendering news fragments... | ||
Draft only -- nothing has been written. | ||
What is seen below is what would be written. | ||
|
||
7.8.9 - 20-01-2001 | ||
================== | ||
|
||
- Adds levitation | ||
|
||
""") | ||
|
||
self.assertEqual(0, result.exit_code, result.output) | ||
self.assertEqual(expected_output, result.output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fortitle_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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