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 support for multiple README files (retrocompatible) #248

Conversation

wagnerluis1982
Copy link
Contributor

@wagnerluis1982 wagnerluis1982 commented Nov 16, 2021

Overview

This code was merged before on #118 and reverted on #235, due to the breaking of Package.readme is renamed to Package.readmes and it is a Tuple[Path] instead of simply Path.

In this PR you will find mostly the same thing of #118 with a few little improvements.

Description

This code reuses the "readme" entry in a way to allow the user to declare in the old way or the new way.

With the changes, the two following declarations are valid:

Single file

readme = "README.rst"

Multiple files

readme = [
    "README.rst",
    "HISTORY.rst"
]

If the user declares files in different formats, the strict validation will issue.

Notice

The class Package suffered an important change: readme was renamed to the plural readmes. Properties for the single form were introduced to ensure retrocompatibility.

Pull Request Check List

Resolves: python-poetry/poetry#873

@wagnerluis1982 wagnerluis1982 marked this pull request as draft November 16, 2021 22:56
wagnerluis1982 added a commit to wagnerluis1982/poetry that referenced this pull request Nov 16, 2021
This makes poetry aware about the upcoming changes in poetry-core in
order to support declaration of multiple readme files in pyproject.toml

Relates python-poetry#873
Enable python-poetry/poetry-core#248
This code reuses the `"readme"` entry in a way to allow the user to
declare in the old way or the new way.

With the changes, the two following declarations are valid:

**Single file**
```toml
readme = "README.rst"
```

**Multiple files**
```toml
readme = [
    "README.rst",
    "HISTORY.rst"
]
```

If the user declares files in different formats, the strict validation
will issue.

NOTICE:

The class `Package` suffered an important change: `readme` was renamed
to the plural `readmes`. Properties for the single form were introduced
to ensure retrocompatibility.
@wagnerluis1982 wagnerluis1982 force-pushed the rework-support-multiple-readme-files branch from cbe43f3 to eb8883e Compare November 19, 2021 00:30
@wagnerluis1982 wagnerluis1982 changed the title Add support for multiple README files (reworked) Add support for multiple README files (retrocompatible) Nov 19, 2021
@wagnerluis1982 wagnerluis1982 marked this pull request as ready for review November 19, 2021 00:38
@abn abn self-requested a review November 20, 2021 13:14
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I'm still not the biggest fan of adding another helper, but we can always move it later during a cleanup of that module.

LGTM after feedback is addressed.

@@ -421,6 +426,14 @@ def validate(cls, config: dict, strict: bool = False) -> Dict[str, List[str]]:
)
)

# Checking types of all readme files (must match)
if "readme" in config and not isinstance(config["readme"], str):
readme_types = [readme_content_type(r) for r in config["readme"]]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
readme_types = [readme_content_type(r) for r in config["readme"]]
readme_types = {readme_content_type(r) for r in config["readme"]}

No reason not to use a set from the start...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I took this decision prematurely, motivated by the fact the set gives non-predictable order (which is important for me in the unit test). I am solving this by adding sorted(readme_types) in the error message value.

# Checking types of all readme files (must match)
if "readme" in config and not isinstance(config["readme"], str):
readme_types = [readme_content_type(r) for r in config["readme"]]
if len(set(readme_types)) > 1:
Copy link
Member

Choose a reason for hiding this comment

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

...which means we can drop the set() call.

import warnings

warnings.warn(
"`readme` is deprecated: you are getting only the first readme file. Please use the plural form `readmes`",
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a period to the end of this sentence.

import warnings

warnings.warn(
"`readme` is deprecated. Please assign a tuple to the plural form `readmes`",
Copy link
Member

Choose a reason for hiding this comment

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

As above.

@wagnerluis1982
Copy link
Contributor Author

I'm still not the biggest fan of adding another helper, but we can always move it later during a cleanup of that module.

LGTM after feedback is addressed.

@neersighted I have to say that I deeply consider about this decision. But the lack of a new helper would mean to store the content type into Package, which doesn't fit IMHO. So I decided to have the helper.

@neersighted neersighted merged commit 6215dd1 into python-poetry:master Nov 21, 2021
@wagnerluis1982 wagnerluis1982 deleted the rework-support-multiple-readme-files branch November 21, 2021 07:13
@finswimmer finswimmer mentioned this pull request Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants