-
Notifications
You must be signed in to change notification settings - Fork 122
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
Interactive create #482
Interactive create #482
Changes from all commits
8647ee0
d90def4
c594476
fda486e
96bb093
13ac5b3
e97b3e2
819638e
49fc0b4
b86c6a0
24d7cb3
c6b5f77
c742497
1718ac1
f071ef8
fe10bfd
e87fcb2
f02e8f6
9211524
f672dbc
9fe507e
49145c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -14,6 +14,9 @@ | |||||
from ._settings import config_option_help, load_config_from_options | ||||||
|
||||||
|
||||||
DEFAULT_CONTENT = "Add your info here" | ||||||
|
||||||
|
||||||
@click.command(name="create") | ||||||
@click.pass_context | ||||||
@click.option( | ||||||
|
@@ -32,30 +35,32 @@ | |||||
) | ||||||
@click.option( | ||||||
"--edit/--no-edit", | ||||||
default=False, | ||||||
default=None, | ||||||
SmileyChris marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
help="Open an editor for writing the newsfragment content.", | ||||||
) # TODO: default should be true | ||||||
) | ||||||
@click.option( | ||||||
"-c", | ||||||
"--content", | ||||||
type=str, | ||||||
default="Add your info here", | ||||||
default=DEFAULT_CONTENT, | ||||||
help="Sets the content of the new fragment.", | ||||||
) | ||||||
@click.argument("filename") | ||||||
@click.argument("filename", default="") | ||||||
def _main( | ||||||
ctx: click.Context, | ||||||
directory: str | None, | ||||||
config: str | None, | ||||||
filename: str, | ||||||
edit: bool, | ||||||
edit: bool | None, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not leave It looks like we are using But maybe it's better to have the explicit default value defined in this way.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only set to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
content: str, | ||||||
) -> None: | ||||||
""" | ||||||
Create a new news fragment. | ||||||
|
||||||
Create a new news fragment called FILENAME or pass the full path for a file. | ||||||
Towncrier has a few standard types of news fragments, signified by the file extension. | ||||||
If FILENAME is not provided, you'll be prompted to create it. | ||||||
|
||||||
Towncrier has a few standard types of news fragments, signified by the file | ||||||
extension. | ||||||
|
||||||
\b | ||||||
These are: | ||||||
|
@@ -76,14 +81,34 @@ def __main( | |||||
directory: str | None, | ||||||
config_path: str | None, | ||||||
filename: str, | ||||||
edit: bool, | ||||||
edit: bool | None, | ||||||
content: str, | ||||||
) -> None: | ||||||
""" | ||||||
The main entry point. | ||||||
""" | ||||||
base_directory, config = load_config_from_options(directory, config_path) | ||||||
|
||||||
filename_ext = "" | ||||||
if config.create_add_extension: | ||||||
ext = os.path.splitext(config.filename)[1] | ||||||
if ext.lower() in (".rst", ".md"): | ||||||
filename_ext = ext | ||||||
|
||||||
if not filename: | ||||||
prompt = "Issue number" | ||||||
# Add info about adding orphan if config is set. | ||||||
if config.orphan_prefix: | ||||||
prompt += f" (`{config.orphan_prefix}` if none)" | ||||||
issue = click.prompt(prompt) | ||||||
fragment_type = click.prompt( | ||||||
"Fragment type", | ||||||
type=click.Choice(list(config.types)), | ||||||
) | ||||||
filename = f"{issue}.{fragment_type}" | ||||||
if edit is None and content == DEFAULT_CONTENT: | ||||||
edit = True | ||||||
|
||||||
file_dir, file_basename = os.path.split(filename) | ||||||
if config.orphan_prefix and file_basename.startswith(f"{config.orphan_prefix}."): | ||||||
# Append a random hex string to the orphan news fragment base name. | ||||||
|
@@ -94,15 +119,18 @@ def __main( | |||||
f"{file_basename[len(config.orphan_prefix):]}" | ||||||
), | ||||||
) | ||||||
if len(filename.split(".")) < 2 or ( | ||||||
filename.split(".")[-1] not in config.types | ||||||
and filename.split(".")[-2] not in config.types | ||||||
filename_parts = filename.split(".") | ||||||
if len(filename_parts) < 2 or ( | ||||||
filename_parts[-1] not in config.types | ||||||
and filename_parts[-2] not in config.types | ||||||
): | ||||||
raise click.BadParameter( | ||||||
"Expected filename '{}' to be of format '{{name}}.{{type}}', " | ||||||
"where '{{name}}' is an arbitrary slug and '{{type}}' is " | ||||||
"one of: {}".format(filename, ", ".join(config.types)) | ||||||
) | ||||||
if filename_parts[-1] in config.types and filename_ext: | ||||||
filename += filename_ext | ||||||
|
||||||
if config.directory: | ||||||
fragments_directory = os.path.abspath( | ||||||
|
@@ -135,31 +163,34 @@ def __main( | |||||
) | ||||||
|
||||||
if edit: | ||||||
edited_content = _get_news_content_from_user(content) | ||||||
if edited_content is None: | ||||||
click.echo("Abort creating news fragment.") | ||||||
if content == DEFAULT_CONTENT: | ||||||
content = "" | ||||||
content = _get_news_content_from_user(content) | ||||||
if not content: | ||||||
click.echo("Aborted creating news fragment due to empty message.") | ||||||
ctx.exit(1) | ||||||
content = edited_content | ||||||
|
||||||
with open(segment_file, "w") as f: | ||||||
f.write(content) | ||||||
if config.create_eof_newline and content and not content.endswith("\n"): | ||||||
f.write("\n") | ||||||
|
||||||
click.echo(f"Created news fragment at {segment_file}") | ||||||
|
||||||
|
||||||
def _get_news_content_from_user(message: str) -> str | None: | ||||||
initial_content = ( | ||||||
"# Please write your news content. When finished, save the file.\n" | ||||||
"# In order to abort, exit without saving.\n" | ||||||
'# Lines starting with "#" are ignored.\n' | ||||||
) | ||||||
initial_content += f"\n{message}\n" | ||||||
def _get_news_content_from_user(message: str) -> str: | ||||||
initial_content = """ | ||||||
# Please write your news content. Lines starting with '#' will be ignored, and | ||||||
# an empty message aborts. | ||||||
""" | ||||||
if message: | ||||||
initial_content = f"{message}\n{initial_content}" | ||||||
content = click.edit(initial_content) | ||||||
if content is None: | ||||||
return None | ||||||
return message | ||||||
all_lines = content.split("\n") | ||||||
lines = [line.rstrip() for line in all_lines if not line.lstrip().startswith("#")] | ||||||
return "\n".join(lines) | ||||||
return "\n".join(lines).strip() | ||||||
|
||||||
|
||||||
if __name__ == "__main__": # pragma: no cover | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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). | ||
SmileyChris marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is feature, or a backward compatiblity break :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I remember there were other disussions about this |
||
|
||
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these 2 new config options have the default set to False, for backward compatibility ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!