Skip to content

title_format overrides default top line#276

Merged
altendky merged 7 commits intotwisted:masterfrom
altendky:180-altendky
Dec 19, 2020
Merged

title_format overrides default top line#276
altendky merged 7 commits intotwisted:masterfrom
altendky:180-altendky

Conversation

@altendky
Copy link
Copy Markdown
Member

@altendky altendky commented Oct 14, 2020

Fixes #180

@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 4, 2020

Codecov Report

Merging #276 (f04b57b) into master (96178dc) will increase coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
+ Coverage   95.57%   95.86%   +0.29%     
==========================================
  Files          20       20              
  Lines        1085     1089       +4     
  Branches      105      104       -1     
==========================================
+ Hits         1037     1044       +7     
+ Misses         27       25       -2     
+ Partials       21       20       -1     
Impacted Files Coverage Δ
src/towncrier/_builder.py 94.28% <ø> (ø)
src/towncrier/test/test_write.py 100.00% <ø> (ø)
src/towncrier/build.py 93.24% <100.00%> (+3.63%) ⬆️
src/towncrier/test/test_format.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 96178dc...f04b57b. Read the comment docs.

@altendky altendky requested a review from adiroiban December 18, 2020 23:12
Copy link
Copy Markdown
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.

All good. Thanks. Only minor comments.


definitions = OrderedDict()

expected_output = u"""A custom top line
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to make the code a bit easier to read I would prefer to have the expect_outpupt" define just before the call to self.assertEqual(output, expected_output)`

Comment on lines +362 to +365
output = render_fragments(
template,
None,
"A custom top line",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also. to make all these render_fragments easier to read, it woud be nice to call them with named arguments for all argumetns and not only

Suggested change
output = render_fragments(
template,
None,
"A custom top line",
output = render_fragments(
template=template,
issue_format=None,
top_line="A custom top line",


expected_output = u"""A custom top line
=================
""" # NOQA
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

minor comment. Why do we need no qa here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm guessing it was the indentation? Switched to dedent().

Comment on lines +347 to +350
self.maxDiff = None

fragments = {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that maxDiff ended up after a debugging session :)

Suggested change
self.maxDiff = None
fragments = {}

"towncrier", "templates/default.rst"
).decode("utf8")

fragments = split_fragments(fragments, definitions)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe use this ... in this way e have a hint that we don't care about fragments or definitions in this test and you don't need to look and see where and how they are created and how they are checked at the end.

Suggested change
fragments = split_fragments(fragments, definitions)
fragments = split_fragments(fragments={}, definitions=OrderedDict())

But for this tests it would be interesting to also have at least one definition and one fragment to increase the coverage and make it easy to compare the output for the case in which you run without a custom topline.

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.

Rendering issue with release candidate (19.9.0rc1)

3 participants