Skip to content
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

title_format overrides default top line #276

Merged
merged 7 commits into from
Dec 19, 2020

Conversation

altendky
Copy link
Member

@altendky altendky commented Oct 14, 2020

Fixes #180

@codecov-io
Copy link

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.

Copy link
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
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
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
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
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
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
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