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

Config location separate from directory containing news file and fragments #548

Merged
merged 7 commits into from
Oct 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/cli.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ The following options can be passed to all of the commands that explained below:

.. option:: --dir PATH

Build fragment in ``PATH``.
The command is executed relative to ``PATH``.
Copy link
Member

Choose a reason for hiding this comment

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

Can we also have this information as part of the built-in CLI help system ?

this would need updating the @click.options help text for build.

https://github.com/twisted/towncrier/pull/548/files#diff-3576f35f5faa5ded14cd2b3db52e19829e34270f7957ebdf765870dc6f305903R68-R74


In an ideal case, we would only write the docs for click and have it automatically exported in RST format.


Same comment is valid for create and check sub-commands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add this to the options. Have you looked into using this sphinx plugin to extract the documentation automatically?

Copy link
Member

Choose a reason for hiding this comment

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

That's the plan. It just needs someone to implement it into the current build system :)

For instance with the default config news fragments are checked and added in ``PATH/newsfragments`` and the news file is built in ``PATH/NEWS.rst``.

Default: current directory.

Expand Down
3 changes: 2 additions & 1 deletion docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,13 @@ Top level keys
The directory storing your news fragments.

For Python projects that provide a ``package`` key, the default is a ``newsfragments`` directory within the package.
Otherwise the default is a ``newsfragments`` directory relative to the configuration file.
Otherwise the default is a ``newsfragments`` directory relative to either the directory passed as ``--dir`` or (by default) the configuration file.

``filename``
The filename of your news file.

``"NEWS.rst"`` by default.
Its location is determined the same way as the location of the directory storing the news fragments.

``template``
Path to the template for generating the news file.
Expand Down
1 change: 1 addition & 0 deletions docs/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Narrative

tutorial
markdown
monorepo


Reference
Expand Down
52 changes: 52 additions & 0 deletions docs/monorepo.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
Multiple Projects Share One Config (Monorepo)
=============================================

Several projects may have independent release notes with the same format.
For instance packages in a monorepo.
Here's how you can use towncrier to set this up.

Below is a minimal example:

.. code-block:: text

repo
├── project_a
│ ├── newsfragments
│ │ └── 123.added
│ ├── project_a
│ │ └── __init__.py
│ └── NEWS.rst
├── project_b
│ ├── newsfragments
│ │ └── 120.bugfix
│ ├── project_b
│ │ └── __init__.py
│ └── NEWS.rst
└── towncrier.toml

The ``towncrier.toml`` looks like this:

.. code-block:: toml

[tool.towncrier]
# It's important to keep these config fields empty
# because we have more than one package/name to manage.
package = ""
name = ""

Now to add a fragment:

.. code-block:: console

towncrier create --config towncrier.toml --dir project_a 124.added

This should create a file at ``project_a/newsfragments/124.added``.

To build the news file for the same project:

.. code-block:: console

towncrier build --config towncrier.toml --dir project_a --version 1.5

Note that we must explicitly pass ``--version``, there is no other way to get the version number.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe create a GitHub issue for this.
I think that if we want to support mono-repos, this should be a 1st class feature... so in the ideal case, the version could be extracted

I don't know how to do it... as we are missing the package name info... so it's not easy to know for which package to extract the version.

But maybe it's best to just have the issue created and linked from here.
If someone else wants this feature, they can send suggestion or PRs for how to fix this.

The sentence from below, reads like a bug description :)

The ``towncrier.toml`` can only contain one version number and the ``package`` field is of no use for the same reason.
4 changes: 3 additions & 1 deletion src/towncrier/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,9 @@ def __main(
click.echo("Finding news fragments...", err=to_err)

if config.directory is not None:
fragment_base_directory = os.path.abspath(config.directory)
fragment_base_directory = os.path.abspath(
os.path.join(base_directory, config.directory)
)
fragment_directory = None
else:
fragment_base_directory = os.path.abspath(
Expand Down
10 changes: 5 additions & 5 deletions src/towncrier/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,7 @@ def __main(
)
sys.exit(0)

files = {
os.path.normpath(os.path.join(base_directory, path)) for path in files_changed
}
files = {os.path.abspath(path) for path in files_changed}

click.echo("Looking at these files:")
click.echo("----")
Expand All @@ -109,7 +107,9 @@ def __main(
sys.exit(0)

if config.directory:
fragment_base_directory = os.path.abspath(config.directory)
fragment_base_directory = os.path.abspath(
os.path.join(base_directory, config.directory)
)
fragment_directory = None
else:
fragment_base_directory = os.path.abspath(
Expand All @@ -118,7 +118,7 @@ def __main(
fragment_directory = "newsfragments"

fragments = {
os.path.normpath(path)
os.path.abspath(path)
for path in find_fragments(
fragment_base_directory,
config.sections,
Expand Down
2 changes: 2 additions & 0 deletions src/towncrier/newsfragments/548.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Initial support was added for monorepo-style setup.
One project with multiple independent news files stored in separate sub-directories, that share the same towncrier config.
27 changes: 27 additions & 0 deletions src/towncrier/test/test_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,33 @@ def test_in_different_dir_config_option(self, runner):
self.assertEqual(0, result.exit_code)
self.assertTrue((project_dir / "NEWS.rst").exists())

@with_isolated_runner
def test_in_different_dir_with_nondefault_newsfragments_directory(self, runner):
"""
Using the `--dir` CLI argument, the NEWS file can
be generated in a sub-directory from fragments
that are relatives to that sub-directory.

The path passed to `--dir` becomes the
working directory.
"""
Path("pyproject.toml").write_text(
"[tool.towncrier]\n" + 'directory = "changelog.d"\n'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also add an explicit empty package config here.
My understanding, is that this is required for this usecase.

)
Path("foo/foo").mkdir(parents=True)
Path("foo/foo/__init__.py").write_text("")
Path("foo/changelog.d").mkdir()
Path("foo/changelog.d/123.feature").write_text("Adds levitation")
self.assertFalse(Path("foo/NEWS.rst").exists())

result = runner.invoke(
cli,
("--yes", "--config", "pyproject.toml", "--dir", "foo", "--version", "1.0"),
Copy link
Member

Choose a reason for hiding this comment

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

I see normpath was previously used.

Maybe for the test, we can go wild, and add something like this, to make sure dir still works when it is passed as a non-normalized path.

Suggested change
("--yes", "--config", "pyproject.toml", "--dir", "foo", "--version", "1.0"),
("--yes", "--config", "pyproject.toml", "--dir", "./foo/../foo/", "--version", "1.0"),

also, do we need to pass --config=pyproject.toml here ? I was thinking that this is already the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for "wild" suggestion, I'll try it out!

We must explicitly pass --config here because otherwise we will search for pyproject.toml inside the directory provided by --dir :)

)

self.assertEqual(0, result.exit_code)
self.assertTrue(Path("foo/NEWS.rst").exists())

@with_isolated_runner
def test_no_newsfragment_directory(self, runner):
"""
Expand Down
98 changes: 98 additions & 0 deletions src/towncrier/test/test_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,3 +297,101 @@ def test_get_default_compare_branch_fallback(self):

self.assertEqual("origin/master", branch)
self.assertTrue(w[0].message.args[0].startswith('Using "origin/master'))

@with_isolated_runner
def test_in_different_dir_with_nondefault_newsfragments_directory(self, runner):
"""
It can check the fragments located in a sub-directory
that is specified using the `--dir` CLI argument.
"""
main_branch = "main"
Path("pyproject.toml").write_text(
# Important to customize `config.directory` because the default
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment.

Maybe something like

For monorepos, the directory is used not as relative to the configuration file, but as relative to the --dir CLI argument.

Copy link
Contributor Author

@iliakur iliakur Oct 18, 2023

Choose a reason for hiding this comment

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

Yea, hmm, this must really be complicated cuz we're having to go over it a 3rd time now (I mentioned this in the PR description and in a comment about bug vs feature criteria).

Let me know if the following is clearer.

  1. We compute the fragments base dir path differently depending on whether config.directory is set or not. References: build, check and create.
  2. The implementation for the case when config.directory is not set is the same across all 3 commands. This implementation supports the usecase we care about in this PR.
  3. The problems start in the other case, however. create implementation already supports the usecase in this PR whereas build and check have to be adjusted.
  4. Following the logic of testing "wilder" scenarios, and also to make sure I'm testing my changes, I'm setting config.directory.

Longer-term I think it makes sense to figure out how to merge these implementations for ease of maintenance. I didn't have time to do so for this PR.

# already supports this scenario.
"[tool.towncrier]\n"
+ 'directory = "changelog.d"\n'
)
subproject1 = Path("foo")
(subproject1 / "foo").mkdir(parents=True)
(subproject1 / "foo/__init__.py").write_text("")
(subproject1 / "changelog.d").mkdir(parents=True)
(subproject1 / "changelog.d/123.feature").write_text("Adds levitation")
initial_commit(branch=main_branch)
call(["git", "checkout", "-b", "otherbranch"])

# We add a code change but forget to add a news fragment.
write(subproject1 / "foo/somefile.py", "import os")
Copy link
Member

Choose a reason for hiding this comment

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

maybe also write a file like ./changelog.d/234.feature that is relative to the config file.

I think that the main goal of this tests it to make sure that --dir is used as the "working directory"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I understand correctly that we don't want ./changelog.d/234.feature to influence the changelog generation?

commit("add a file")
result = runner.invoke(
towncrier_check,
(
"--config",
"pyproject.toml",
"--dir",
str(subproject1),
"--compare-with",
"main",
),
)

self.assertEqual(1, result.exit_code)
self.assertTrue(
result.output.endswith("No new newsfragments found on this branch.\n")
)

# We add the news fragment.
fragment_path = (subproject1 / "changelog.d/124.feature").absolute()
write(fragment_path, "Adds gravity back")
commit("add a newsfragment")
result = runner.invoke(
towncrier_check,
("--config", "pyproject.toml", "--dir", "foo", "--compare-with", "main"),
)

self.assertEqual(0, result.exit_code, result.output)
self.assertTrue(
result.output.endswith("Found:\n1. " + str(fragment_path) + "\n"),
(result.output, str(fragment_path)),
)

# We add a change in a different subproject without a news fragment.
# Checking subproject1 should pass.
subproject2 = Path("bar")
(subproject2 / "bar").mkdir(parents=True)
(subproject2 / "changelog.d").mkdir(parents=True)
write(subproject2 / "bar/somefile.py", "import os")
commit("add a file")
result = runner.invoke(
towncrier_check,
(
"--config",
"pyproject.toml",
"--dir",
subproject1,
"--compare-with",
"main",
),
)

self.assertEqual(0, result.exit_code, result.output)
self.assertTrue(
result.output.endswith("Found:\n1. " + str(fragment_path) + "\n"),
(result.output, str(fragment_path)),
)

# Checking subproject2 should result in an error.
result = runner.invoke(
towncrier_check,
(
"--config",
"pyproject.toml",
"--dir",
subproject2,
"--compare-with",
"main",
),
)
self.assertEqual(1, result.exit_code)
self.assertTrue(
result.output.endswith("No new newsfragments found on this branch.\n")
)
34 changes: 34 additions & 0 deletions src/towncrier/test/test_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,37 @@ def test_create_orphan_fragment_custom_prefix(self, runner: CliRunner):
self.assertEqual(len(change.stem), 11)
# Check the remainder are all hex characters.
self.assertTrue(all(c in string.hexdigits for c in change.stem[3:]))

@with_isolated_runner
def test_in_different_dir_with_nondefault_newsfragments_directory(self, runner):
"""
When the `--dir` CLI argument is passed,
it will create a new file in directory that is
created by combining the `--dir` value
with the `directory` option from the configuration
file.
"""
Path("pyproject.toml").write_text(
# Important to customize `config.directory` because the default
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment here.

Maybe, it's important to note that this is a configured using a sub-directory that is later combined with the CLI argument.

Also, maybe it's also important to pass an empty project value for this test

# already supports this scenario.
"[tool.towncrier]\n"
+ 'directory = "changelog.d"\n'
)
Path("foo/foo").mkdir(parents=True)
Path("foo/foo/__init__.py").write_text("")

result = runner.invoke(
_main,
(
"--config",
"pyproject.toml",
"--dir",
"foo",
"--content",
"Adds levitation.",
"123.feature",
),
)

self.assertEqual(0, result.exit_code)
self.assertTrue(Path("foo/changelog.d/123.feature").exists())
Loading