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

Configurable Description-Content-Type #360

Merged
merged 5 commits into from
Oct 8, 2020
Merged

Conversation

clbarnes
Copy link
Contributor

@clbarnes clbarnes commented Oct 7, 2020

No validation is performed.
The defaults are unchanged (and inconsistent with setuptools/PyPI): that is, None if there is no readme, GFM if there is a readme and no explicit content type.

Basic test for checking metadata gets through.

See #247

No validation is performed.
The defaults are unchanged (and inconsistent with setuptools/PyPI).

Basic test for checking metadata gets through.
@clbarnes clbarnes changed the title Configurable description_content_type Configurable Description-Content-Type Oct 7, 2020
src/metadata.rs Outdated
let actual = metadata.to_file_contents();

if actual.trim() != expected.trim() {
panic!(
Copy link
Member

Choose a reason for hiding this comment

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

You can pass a message and formatter options as third and following parameters to assert_eq (example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, that was a leftover from a slightly different implementation, I'll change that.

src/metadata.rs Outdated
version = "0.1.0"
description = "A test project"
homepage = "https://example.org"
readme = "readme.md"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be readme.rst?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be but it doesn't have to be, as this test just needs to prove that the metadata gets through. I'll fix it for clarity, though.

Copy link
Contributor Author

@clbarnes clbarnes Oct 8, 2020

Choose a reason for hiding this comment

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

On second look, it doesn't actually matter because the substring is replaced by the path of the temporary file created to write the readme contents into; I've changed the name to make that more clear, as well as explicitly checking for some expected hardcoded values.

@konstin
Copy link
Member

konstin commented Oct 8, 2020

Thank you for the pull request!

I like adding the description_content_type option, but could we try to guess it based on the file extension? So set it to markdown for Readme.md and to rst for Readme.rst?

@clbarnes
Copy link
Contributor Author

clbarnes commented Oct 8, 2020

could we try to guess it based on the file extension?

Should be easy enough!

... if not explicitly given.

BREAKING: when no type is given AND the file does not have a recognised
extension, maturin will now default to plaintext.
This makes more sense, but is technically a breaking change.
@clbarnes
Copy link
Contributor Author

clbarnes commented Oct 8, 2020

Note fef38f8 - where no content type is given, and the file path does not have a known extension, content type defaults to plaintext rather than GFM. I think this makes most sense and the results shouldn't be disastrous (as markdown should be largely readable in plain text), but it is technically a breaking change.

There's a little hole in the testing here because the existing tests write to and read from a file without an extension, so the end-to-end test for checking extensions is a bit tricky. But the filename-parsing logic is tested.

@clbarnes
Copy link
Contributor Author

clbarnes commented Oct 8, 2020

Test failure relates to a missing GLIBC version, which I hope I'm not responsible for 😬

@konstin
Copy link
Member

konstin commented Oct 8, 2020

Note fef38f8 - where no content type is given, and the file path does not have a known extension, content type defaults to plaintext rather than GFM. I think this makes most sense and the results shouldn't be disastrous (as markdown should be largely readable in plain text), but it is technically a breaking change.

Thanks, that's definitely better than before!

Test failure relates to a missing GLIBC version, which I hope I'm not responsible for grimacing

Yep, iirc a recent rustc version dropped support for manylinux1 and I have to move it to manylinux2010.

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Code looks good, thank you for implementing that!

@konstin konstin merged commit e98bc14 into PyO3:master Oct 8, 2020
gustavgransbo added a commit to gustavgransbo/networkg that referenced this pull request Dec 23, 2020
Using a newer version of maturin to properly set
the description content type meta on built package.

This functionality was intorduced in:
PyO3/maturin#360
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