Interactive create #482

SmileyChris · 2023-02-21T22:15:19Z

Description

If no filename is given when doing towncrier create, interactively ask for the issue number and fragment type (and then launch an interactive editor for the newsfragment content).

$ towncrier create
Issue number (`+` for none): 482
Fragment type (feature, bugfix, doc, removal, misc): feature
($EDITOR opened here)
Created news fragment at /home/chris/dev/towncrier/src/towncrier/newsfragments/482.feature.rst

Reworded interactive edit comment

The comment given to the editor has been reworded to use the similar format as a git commit interactive edit, with the empty space above rather than below:

█
# Please write your news content. Lines starting with '#' will be ignored, and
# an empty message aborts.

Default news fragment filename extension

If the file extension of the filename configuration setting ends in .rst or .md, towncrier create will append this extension to newsfragments it creates (if the filename didn't provide an explicit extension).

$ towncrier create 1.feature
Created news fragment at /home/chris/dev/towncrier/src/towncrier/newsfragments/1.feature.rst
$ towncrier create 2.feature.txt
Created news fragment at /home/chris/dev/towncrier/src/towncrier/newsfragments/2.feature.txt

This is configurable with a new create_add_extension setting (true by default).

Add an empty line to the end of news fragments

An empty line is now added to the end of news fragment content, whether entered by the command line option or interactively.
This is configurable with a new create_eof_newline setting (true by default).

Checklist

Make sure changes are covered by existing or new tests.
For at least one Python version, make sure local test run is green.
Create a file in src/towncrier/newsfragments/. Describe your
change and include important information. Your change will be included in the public release notes.
Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
Ensure docs/tutorial.rst is still up-to-date.
If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

for more information, see https://pre-commit.ci

codecov · 2023-02-21T22:21:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (70da86f) to head (e97b3e2).
Report is 54 commits behind head on trunk.

❗ Current head e97b3e2 differs from pull request most recent head 49145c7. Consider uploading reports for the commit 49145c7 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##            trunk      #482      +/-   ##
===========================================
+ Coverage   99.84%   100.00%   +0.15%     
===========================================
  Files          13        13              
  Lines         631       654      +23     
  Branches      146       156      +10     
===========================================
+ Hits          630       654      +24     
+ Partials        1         0       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adiroiban · 2023-02-22T12:39:04Z

Thanks Chris for this nice PR.
I will try to find some time to review this...but I am very busy this need and next week.
I hope someone else will have time to review it.
If someone left unreviewed don't hesitate to ping me and I will to review.

At a first review, my initial comment was to move the configuration changes into a separate PR and focus this PR only on the interactive part.
In this way, it might be easier to merge one part of this PR, without waiting for other part that might need more work/feedback.

But I am also ok to review this together. It's just that it will take more time to review.

Thanks again for your help.

SmileyChris · 2023-02-28T23:57:09Z

But I am also ok to review this together. It's just that it will take more time to review.

Yes, I threw in a bit more than I should have... but it was all intertwined and it was too messy to provide them as individual PRs. If any area becomes contentious, I'm happy to remove it from this PR.

Preemptively defending the secondary changes:

Interactive edit rewording: this should be pretty straight forward, it just brings things into line with a git interactive commit, and having the empty line first is a nice advantage to allow typing without navigation in most editors.
Default news fragment: this is a completely optional addition, but one that fit nicely with recent discussions on file extensions we were having. Having a .rst (or .md) extension makes the experience nicer for users who have an editor that styles content based on file type.
Adding empty lines to fragments: functionally it doesn't change anything with generated output, but it is good practice (and without, fragments technically not even technically text files according to IEEE Std 1003.1-2001: A text file consists of a series of lines, each of which requires a terminating newline character). A common pre-commit hook adds a newline the end of files.

adiroiban · 2023-03-02T13:19:14Z

Thanks. I will try to take a look at this next week.

adiroiban · 2023-10-23T09:13:25Z

Sorry Chris for this late review.
It was still marked as unread... but went at the bottom of my github notifications list.

If you are still interested in this. I think that as long as the tests are green, this can be merged.

I don't know whey I will have time to test this.

adiroiban

Sorry for the late review. and many thanks for the PR. it was easy to follow.

I did a quick code review.

I didn't had time to get the code and run manual tests.

Please see my inline comments and let me know if they make sense.

I think the changes are ok, but it's important to have better release notes for the changes here.

it looks like besides interactive create, this PR also adds 2 new features and breaks at least 1 backward compatibility :)

We can merge this as it is, as long as we document the changes as part of the release notes.

src/towncrier/create.py

adiroiban · 2023-10-23T09:19:27Z

src/towncrier/create.py

 def _main(
 ctx: click.Context,
 directory: str | None,
 config: str | None,
 filename: str,
- edit: bool,
+ edit: bool | None,


Why not leave edit as always being boolean ?

It looks like we are using None here as a placeholder for the default value.

But maybe it's better to have the explicit default value defined in this way.

Suggested change

edit: bool | None,

edit: bool = True,

This is only set to True only if the content is still the default content. If a user specifies their own content then, we don't want to default to triggering editing mode. But by leaving it None, it makes it possible to provide custom content and still explicitly choose to enable editing mode with this flag.

We can have it like this... it looks like we are hijacking this variable :)

We should have a separate flag to signal the other use case.

It's the same behaviour as any of the other cli settings that also have a configuration setting. If None, fall back to the configuration, otherwise use the value as provided.

src/towncrier/create.py

src/towncrier/newsfragments/482.feature.rst

adiroiban · 2023-10-23T09:23:23Z

src/towncrier/test/test_create.py

+ additional_args=["-c", content_line, "--no-eof-newline"],
+ eof_newline=False,
+ )
+
 def test_message_and_edit(self):
 """
 When creating a new message, a initial content can be passed via
 the command line and continue modifying the content by invoking the
 text editor.


Maybe add a comment that the conten is pass verbatim and no extra newline or processed is done.

Because that comment would be incorrect. The newline option is still processed after it's received back from the editor.

This is specifically testing the case mentioned elsewhere when a user provides custom content but still chooses to edit their content first.

OMG :) Are people using this feature ?

I am ok with that, we can have it.

It looks so complicated :) ... but I have never felt the need for towncrier create

For me it's always easier to just create the file directly in my text editor, without calling towncrier.

src/towncrier/test/test_create.py

… feature/interactive-create

adiroiban

Thanks for the update.

I don't have much time for this reaview...and I don't really understand the need for towncrier create ... in general :)

So, I am not the best person to review this.

But we should try go get this merged.

As long as it has docs + tests, it should be ok.

My comments are about:

backward compatibilty , related to the default values of the new config options
adding yet another CLI argument to towncrier create ... maybe having the value in config file is enough

I hope @hynek or @altendky can take a look and see if we need the extra CLI argument and it it's ok to introduce this backward compatiblity change.

Thanks again

I am using a towncrier version from 2017 ...and I am happy with that. So I don't really care.

I am just trying to maintain this, as part of my release management work for twisted/twisted

adiroiban · 2024-04-24T13:37:44Z

src/towncrier/create.py

 def _main(
 ctx: click.Context,
 directory: str | None,
 config: str | None,
 filename: str,
- edit: bool,
+ edit: bool | None,


We can have it like this... it looks like we are hijacking this variable :)

We should have a separate flag to signal the other use case.

adiroiban · 2024-04-24T13:39:44Z

src/towncrier/_settings/load.py

@@ -52,6 +52,8 @@ class Config:
 wrap: bool = False
 all_bullets: bool = True
 orphan_prefix: str = "+"
+ create_eof_newline: bool = True
+ create_add_extension: bool = True


Should these 2 new config options have the default set to False, for backward compatibility ?

It depends if backwards compatibility is more important than (imo) more sensible defaults. I'll leave that decision to the maintainers to make a call on.

I am not using the create command... so I don't know what to say.

In general, the rule is to keep backward compatibiliy. Any exception should be explicitly raised and accepted on a case by case basis.

We don't have a separate backward compatiblity policy for towncrier ... so I go with the general Twisted org policy https://docs.twisted.org/en/stable/development/compatibility-policy.html

I am happy to have a separate policy for towncrier.
Since towncrie is a dev tool, I think we can be more releaxed.

At the same time, I don't like when a CI pipeline breaks after an update

I guess that towncrier create is not used as part of any automation... so we should be ok.

If nobody else is -1 on this default, I will merge is at it is.

I am happy with the new defaults.

Thanks!

src/towncrier/create.py

adiroiban · 2024-04-24T13:47:07Z

src/towncrier/test/test_create.py

+ additional_args=["-c", content_line, "--no-eof-newline"],
+ eof_newline=False,
+ )
+
 def test_message_and_edit(self):
 """
 When creating a new message, a initial content can be passed via
 the command line and continue modifying the content by invoking the
 text editor.


OMG :) Are people using this feature ?

I am ok with that, we can have it.

It looks so complicated :) ... but I have never felt the need for towncrier create

For me it's always easier to just create the file directly in my text editor, without calling towncrier.

adiroiban · 2024-04-24T13:50:15Z

src/towncrier/newsfragments/482.feature.rst

+
+Now by default, when creating a fragment it will be appended with the ``filename`` option's extension (unless an extension is explicitly provided). For example, ``towncrier create 123.feature`` will create ``news/123.feature.rst``. This can be changed in configuration file by setting `add_extension = false`.
+
+A new line is now added by default to the end of the fragment contents. This can be reverted in the configuration file by setting `add_newline = false` or explicitly set with the ``create`` command line flags ``--eof-newline/--no-eof-newline``.


I am ok with this new default value... but maybe it should be placed into a separate section ... as it looks like a backward incompatible change.

As in a separate fragment? I'm not sure where else you mean this should go.

Usualy backward incopatible changes are announced into 482.removal.rst This is the "Deprecation and removal" category.

I know that "removal" is a bad name... but this is what we have.

There are a log of bad defaults and bad naming conventions in towncrier :(

bad as in non intuitive

adiroiban · 2024-04-24T13:50:58Z

src/towncrier/newsfragments/482.feature.rst

@@ -0,0 +1,5 @@
+If no filename is given when doing ``towncrier`` create, interactively ask for the issue number and fragment type (and then launch an interactive editor for the fragment content).
+
+Now by default, when creating a fragment it will be appended with the ``filename`` option's extension (unless an extension is explicitly provided). For example, ``towncrier create 123.feature`` will create ``news/123.feature.rst``. This can be changed in configuration file by setting `add_extension = false`.


Not sure if this is feature, or a backward compatiblity break :)

It's a change in the filename but since the file is parsed by towncrier in exactly the same way, I don't think it's really backwards incompatible

OK. Thanks. Fine by me.

I am not sure that adding release notes in this way, in a single fragment, does what we want.

I remember that towncrier will create one list item per file

On my project, as a hack I have 482-1.feature.rst as a way to add another item in the notes...but I think that we need better support here

I remember there were other disussions about this

…ig option is enough.

adiroiban

Thanks.

All good.

The only think blocking this is moving the backward incomaptible change into a .removal fragment

I will wait a bit to see if anyone else has anything agains this backward incopatible change and all is ok, this can be merged.

thanks again!

adiroiban · 2024-04-26T10:27:39Z

@SmileyChris I think that you are already in the top 5 contributors of this repo for the last few years, so I consider you one of the de-facto maintainers

I sent you an invite to a team that has write access to the repo.
This will get you write access and issue triage access. You should be able to add tags and update issues

adiroiban · 2024-04-28T12:30:01Z

I found a bit of time today, so I have triggered the auto-merge.

I am not sure when I will have time to check this.

If anyone is -1 on these changes, we can still revert them before the next public release.

Thanks for all your help!

SmileyChris and others added 3 commits February 22, 2023 11:03

Interactive create when no filename is passed

8647ee0

Configurable eof newline

d90def4

[pre-commit.ci] auto fixes from pre-commit.com hooks

c594476

for more information, see https://pre-commit.ci

SmileyChris added 3 commits February 22, 2023 11:34

Add changes

fda486e

More tests for file extensions

96bb093

mypy fix

13ac5b3

SmileyChris marked this pull request as ready for review February 22, 2023 00:53

SmileyChris requested a review from a team as a code owner February 22, 2023 00:53

Test interactive create for orphan news fragments

e97b3e2

SmileyChris added 6 commits May 8, 2023 09:36

Interactive create when no filename is passed

819638e

Configurable eof newline

49fc0b4

Add changes

b86c6a0

More tests for file extensions

24d7cb3

mypy fix

c6b5f77

Test interactive create for orphan news fragments

c742497

SmileyChris force-pushed the feature/interactive-create branch from e97b3e2 to c742497 Compare May 7, 2023 21:57

SmileyChris mentioned this pull request Jun 5, 2023

Remove incremental #491

Closed

7 tasks

Merge branch 'trunk' into feature/interactive-create

1718ac1

Merge branch 'trunk' into feature/interactive-create

f071ef8

adiroiban requested changes Oct 23, 2023

View reviewed changes

SmileyChris added 4 commits April 24, 2024 13:56

Merge branch 'trunk' into feature/interactive-create

fe10bfd

Document two other new features in the newsfragment

e87fcb2

Merge remote-tracking branch 'origin/feature/interactive-create' into…

f02e8f6

… feature/interactive-create

Test improvements

9211524

SmileyChris requested a review from adiroiban April 24, 2024 02:36

Fix test to use default extension

f672dbc

adiroiban reviewed Apr 24, 2024

View reviewed changes

remove obscure --eof-newline/--no-eof-newline cli option. Have a conf…

9fe507e

…ig option is enough.

adiroiban approved these changes Apr 26, 2024

View reviewed changes

Merge branch 'trunk' into feature/interactive-create

49145c7

adiroiban enabled auto-merge (squash) April 28, 2024 12:29

adiroiban merged commit dd41869 into twisted:trunk Apr 28, 2024
15 checks passed

SmileyChris mentioned this pull request May 16, 2024

Provide interactive prompts to create fragments with the correct names #535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactive create #482

Interactive create #482

SmileyChris commented Feb 21, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading

adiroiban commented Feb 22, 2023

SmileyChris commented Feb 28, 2023 •

edited

Loading

adiroiban commented Mar 2, 2023

adiroiban commented Oct 23, 2023

adiroiban left a comment

adiroiban Oct 23, 2023

SmileyChris Apr 24, 2024

adiroiban Apr 24, 2024

SmileyChris Apr 25, 2024

adiroiban Oct 23, 2023

SmileyChris Apr 24, 2024

SmileyChris Apr 24, 2024

adiroiban Apr 24, 2024

adiroiban left a comment

adiroiban Apr 24, 2024

adiroiban Apr 24, 2024

SmileyChris Apr 25, 2024

adiroiban Apr 26, 2024

adiroiban Apr 24, 2024

adiroiban Apr 24, 2024

SmileyChris Apr 25, 2024

adiroiban Apr 26, 2024 •

edited

Loading

adiroiban Apr 24, 2024

SmileyChris Apr 25, 2024

adiroiban Apr 26, 2024

adiroiban left a comment

adiroiban commented Apr 26, 2024 •

edited

Loading

adiroiban commented Apr 28, 2024


		Now by default, when creating a fragment it will be appended with the ``filename`` option's extension (unless an extension is explicitly provided). For example, ``towncrier create 123.feature`` will create ``news/123.feature.rst``. This can be changed in configuration file by setting `add_extension = false`.

		A new line is now added by default to the end of the fragment contents. This can be reverted in the configuration file by setting `add_newline = false` or explicitly set with the ``create`` command line flags ``--eof-newline/--no-eof-newline``.

		@@ -0,0 +1,5 @@
		If no filename is given when doing ``towncrier`` create, interactively ask for the issue number and fragment type (and then launch an interactive editor for the fragment content).

		Now by default, when creating a fragment it will be appended with the ``filename`` option's extension (unless an extension is explicitly provided). For example, ``towncrier create 123.feature`` will create ``news/123.feature.rst``. This can be changed in configuration file by setting `add_extension = false`.

Interactive create #482

Interactive create #482

Conversation

SmileyChris commented Feb 21, 2023 • edited Loading

Description

Reworded interactive edit comment

Default news fragment filename extension

Add an empty line to the end of news fragments

Checklist

codecov bot commented Feb 21, 2023 • edited Loading

Codecov Report

adiroiban commented Feb 22, 2023

SmileyChris commented Feb 28, 2023 • edited Loading

adiroiban commented Mar 2, 2023

adiroiban commented Oct 23, 2023

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

adiroiban commented Apr 26, 2024 • edited Loading

adiroiban commented Apr 28, 2024

SmileyChris commented Feb 21, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading

SmileyChris commented Feb 28, 2023 •

edited

Loading

adiroiban Apr 26, 2024 •

edited

Loading

adiroiban commented Apr 26, 2024 •

edited

Loading