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 docs + slight refactor for num-complex and num-bigint dependencies #1670

Merged
merged 10 commits into from
Jun 11, 2021
Merged

Add docs + slight refactor for num-complex and num-bigint dependencies #1670

merged 10 commits into from
Jun 11, 2021

Conversation

mejrs
Copy link
Member

@mejrs mejrs commented Jun 7, 2021

  • adds docs and examples for the num-complex and num-bigint dependencies
  • also adds a paragraph to the features section of the guide, which was missing them
  • adds nalgebra as a dev-dependency - this is required to test the num-complex example. I really wanted an example that shows using Complex in a linear algebra crate, which is probably what most people using the feature want to do. I don't like adding dependencies, but given that it requires two other crates I would feel uncomfortable not testing it.
  • shuffles around some code:
    • I put the relevant feature-gated code in the same module as the docs
    • I put the code in scr/types/num that depends on the Py_LIMITED/PyPy cfg's in a single block
    • both these things reduce a lot of the cfg littering in that module
    • this does make the diffs a pain to review 😳 sorry!
Rendered

image
image

image
image

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Overall this is a really nice tidy-up, thank you!

My main query is regarding the #[must_use] annotations. I highlighted two but there's a few more. Are they really necessary?

Comment on lines 18 to 22
//! ```toml
//! [dependencies]
//! num-bigint = "0.4"
//! pyo3 = { version = "0.14", features = ["num-bigint"] }
//! ```
Copy link
Member

Choose a reason for hiding this comment

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

Agreed this is useful to have but I worry we'll forget to update the versions...

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is a way to auto update it :/ This is definitely going to be an annoying maintenance burden going forward.

I've seen other crates document this like:

[dependencies]
# replace * with the current pyo3 version
pyo3 = { version = "*", features = ["num-bigint"] }

Copy link
Member

Choose a reason for hiding this comment

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

👍 That seems like a really good idea. Could also say "check pyo3's Cargo.toml" for supported num-bigint version?

... or we could do something fun with an mdbook preprocessor to parse Cargo.toml and put the supported num-bigint version in the guide in a table 🙈

Copy link
Member Author

@mejrs mejrs Jun 7, 2021

Choose a reason for hiding this comment

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

or we could do something fun with an mdbook preprocessor to parse Cargo.toml and put the supported num-bigint version in the guide in a table 🙈

Can we do something dynamically in the docs? I'd really prefer something that can be straight up copy-pasted.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not aware of similar tricks to do dynamic docs, however if you want to go for something dynamic in the guide you might find the existing preprocessor I wrote useful: https://github.com/PyO3/pyo3/blob/main/guide/pyo3_version.py

Copy link
Member

Choose a reason for hiding this comment

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

Since we have other locations where the current version must be substituted, I think the best way is a small (Python) script that replaces everything at once. This can then be run simply as part of the release process.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried a couple of things but didn't find anything that didn't feel like a gross hack. So I've just taken the * approach as above.

Copy link
Member

Choose a reason for hiding this comment

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

What's the "gross hack" with my suggestion?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not, really. I figured out a proc macro that does it by just search+replacing text in a module, but it doesn't work in lib.rs and messes up the [src] links (which is pretty gross).

I don't like the idea of adding things to the release process for something that's at best "nice to have". It's not that big of a deal on its own but these things do add up.

Copy link
Member

Choose a reason for hiding this comment

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

TBH I would be happy to have a python script to automate releasing (at the moment it's done manually by me), but i think that's best in a separate PR.

src/types/complex.rs Outdated Show resolved Hide resolved
src/types/complex.rs Outdated Show resolved Hide resolved
@mejrs
Copy link
Member Author

mejrs commented Jun 8, 2021

I'm not sure how to fix the failing python3.9-x64 ubuntu-latest MSRV test. The problem is that nalgebra won't build on msrv (officially, they only support latest stable).

I see the following solutions:

  • don't test the example 😢
  • put nalgebra as an optional dependency (dev-dependencies can't be optional) and only turn it on when testing on latest stable. I'm not a fan of that showing up as a feature when it doesn't really do anything useful for users.

@davidhewitt
Copy link
Member

Hmm, that's an awkward one.

I think I'd rather not do anything too complicated but agree it'd be nice to test the example. How about ditching nalgebra and writing example to find the complex roots of a quadratic polynomial instead? That's a reasonably simple formula which could be written without needing a third-party dependency.

@mejrs
Copy link
Member Author

mejrs commented Jun 9, 2021

I've ignore'd the example to avoid the problem but I do want to keep it because it shows a good use case for using the feature.

I think that is all 👌

@kngwyu
Copy link
Member

kngwyu commented Jun 10, 2021

If you can afford it, could you please revise the tests to use with_gil?
BTW, thank you for all your efforts. Unfortunately, recently I don't have enough time for OSS, so I'm happy to see the project is supported this nice way.

@mejrs
Copy link
Member Author

mejrs commented Jun 10, 2021

@kngwyu sure!

@davidhewitt
Copy link
Member

Thanks!

@davidhewitt davidhewitt merged commit 05e4bdf into PyO3:main Jun 11, 2021
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.

4 participants