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

feat(settings): Add support to configure fragment types via tables #370

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

danieljmv01
Copy link
Contributor

  • Add support to toml tables for custom fragments,
    with default values.

Resolves: twisted/#369
Signed-off-by: danieljmv01 danieljmv01@gmail.com

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #370 (bda743b) into master (27cd970) will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
+ Coverage   96.85%   97.04%   +0.19%     
==========================================
  Files          20       22       +2     
  Lines        1241     1321      +80     
  Branches      120      126       +6     
==========================================
+ Hits         1202     1282      +80     
  Misses         20       20              
  Partials       19       19              
Impacted Files Coverage Δ
src/towncrier/_settings/__init__.py 100.00% <100.00%> (ø)
src/towncrier/_settings/fragment_types.py 100.00% <100.00%> (ø)
src/towncrier/_settings/load.py 96.20% <100.00%> (ø)
src/towncrier/test/test_settings.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 27cd970...bda743b. Read the comment docs.

@danieljmv01 danieljmv01 force-pushed the feature/369 branch 2 times, most recently from cd87381 to f72252b Compare February 16, 2022 22:31
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.

Many thanks for the changes.

So we can have [tool.towncrier.fragment.chores] or [tool.towncrier.fragment.feat].

I am a bit (just a bit) worried about possible future conflicts.

So, as you think is best.

We can play safe and go with the more verbose [tool.towncrier.fragment.chores] as long as this is documented, it should be fine.

@adiroiban
Copy link
Member

I am ok with any change that makes you happy :) as long as it has tests to prevent any future regressions.

My only concern is that we should try to get it right, so that in the future we will not have to also deprecate this method :)

So, maybe play it safe and go with [tool.towncrier.fragment.chores]
It's bit more verbose, but you only define this once.
And it should be more explicit.

Thanks again

@adiroiban
Copy link
Member

So, maybe we should go with something like this

[tool.towncrier]
package="foo"
newsfile="news.rst"

[tool.towncrier.fragment.chores]
name= "foo"

It's more verbose, but we don't risk any colision.
I am not very happy about the extra fragment_types list.

I think that if we place everything inside tool.towncrier.fragment.* they can be automatically discovered.

@danieljmv01
Copy link
Contributor Author

So, maybe we should go with something like this

[tool.towncrier]
package="foo"
newsfile="news.rst"

[tool.towncrier.fragment.chores]
name= "foo"

It's more verbose, but we don't risk any colision. I am not very happy about the extra fragment_types list.

I think that if we place everything inside tool.towncrier.fragment.* they can be automatically discovered.

Ok, cool. I will use the keys of "tool.towncrier.fragment" mapping to get the fragments instead of using "fragment_types" array then.

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. Many thanks.

I left a few minor comments.

Other than that, I think that this is ready to merge.

* Add support to toml tables for custom fragments,
  with default values.

Resolves: twisted/twisted#369
Signed-off-by: danieljmv01 <danieljmv01@gmail.com>
@adiroiban adiroiban changed the title feat(settings): Add support to tables in toml settings feat(settings): Add support to configure fragment types via tables Feb 25, 2022
@adiroiban adiroiban enabled auto-merge February 25, 2022 10:26
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.

let's merge this.

We need to remove the duplication of docs from README and docs/ , but that can be don in a separate PR.

Thanks!

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.

2 participants