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

Add title_format tests and drop a line #299

Merged
merged 13 commits into from
Dec 20, 2020

Conversation

altendky
Copy link
Member

No description provided.

@codecov-io
Copy link

codecov-io commented Dec 14, 2020

Codecov Report

Merging #299 (fd541c7) into master (5f37912) will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
+ Coverage   95.86%   96.15%   +0.28%     
==========================================
  Files          20       20              
  Lines        1089     1117      +28     
  Branches      104      104              
==========================================
+ Hits         1044     1074      +30     
+ Misses         25       24       -1     
+ Partials       20       19       -1     
Impacted Files Coverage Δ
src/towncrier/test/test_build.py 100.00% <100.00%> (ø)
src/towncrier/build.py 95.94% <0.00%> (+2.70%) ⬆️

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 5f37912...fd541c7. Read the comment docs.

@altendky altendky requested a review from adiroiban December 20, 2020 00:57
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.

Looks good.

I am not sure what is the scope of this PR and why we need it.

I would argue that we don't need to support false so don't add extra test for it... but at the same time we don't need to add extra logic for it.

I would say that only havingtest_title_format_non_empty is enough for this PR.

Thanks!

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.

I see this in the docs

title_format = "{name} {version} ({project_date})" # or false if template includes title

And now I am confused...

I think that we should change README as part of this PR to make it clear how title_format should be used and what is the purpose of setting it to false.

@altendky
Copy link
Member Author

Explanation was certainly lacking. This followed from #164 where it was reported that title_format = false in the configuration file was not working properly. The code had been fixed by 19.9.0rc1 but I didn't see any tests so I figured it was worth adding a test in response to the ticket rather than just closing the ticket.

My understanding is that if you specify a template which includes the title then you should specify title_format = false to avoid doubling up on titles (your template plus the default title format). So, switch this from misc to a doc news fragment, provide an explanation in the readme, and keep both tests?

After we agree on that I'll handle the rest of the details.

@adiroiban
Copy link
Member

My understanding is that if you specify a template which includes the title then you should specify title_format = false to avoid doubling up on titles (your template plus the default title format). So, switch this from misc to a doc news fragment, provide an explanation in the readme, and keep both tests?

OK. I guess that we should have a separate section to explain each option... and not show them as an example with a brief comment.

So, my understanding is:

title_format = "{name} {version} ({project_date})"

Use this option to define format the title for each release.
Available placeholders are:
* name - name of the project
* version - current version of the project
* project_date - date when the version was/is released

Leave it out or set it to empty string to use the default towncrier format.
Set it to `false`  to not have townrier generate a tiltte. For the case in which your custom template is defining its own title.

And yes. the docstring for each test could be verbose and explain each tests case and what functionality is tested.

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.

With better documentation this can be merged.

@altendky altendky merged commit 46c3157 into twisted:master Dec 20, 2020
@altendky altendky deleted the title_format_tests_and_tweak branch December 20, 2020 19:43
Draft only -- nothing has been written.
What is seen below is what would be written.

FooBarBaz 7.8.9 (20-01-2001)
Copy link

Choose a reason for hiding this comment

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

I missed this when reporting the fix worked for me... This is well incorrect, the title should not appear here as per title_format = false above?

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.

4 participants