Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rewrite
module.md
for clarity and add tip on code organization #1693New 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
Rewrite
module.md
for clarity and add tip on code organization #1693Changes from all commits
7018c3f
1d48c90
fdbd0f9
5d900f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This isn't necessary as of version 1.34 of Rust: https://doc.rust-lang.org/rustdoc/documentation-tests.html#using--in-doc-tests.
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.
This is something we picked up on with docs for Pants: for bigger code blocks, preview what the main thing to look for is.
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.
This is said later in the paragraph. It's repetitive and distracting to say here too.
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.
That's explained in function.md. This page is instead focused on modules, and there's value imo in keeping this example short and focused.
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 personally prefer the
#[pyfunction]
approach and think it makes things easier to understand. Better separation of concerns, such that the#[pymodule]
function is solely for FFI registration and has no business logic.I think the example is easier to follow by using
#[pyfunction]
-function.md
still demos#[pyfn]
.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.
Iirc, the plan is to get rid of
#[pyfn]
at some point.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.
That's great! I was going to open an issue proposing that very thing :) I think there's value in having a consolidate way to do things.
Would you like me to open an issue for this? I'd be happy to implement.
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.
Yeah, I don't like having two slightly different ways to do the same thing.
As for an issue, there is one: #694
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.
Completely agree that
#[pyfn]
should go. The syntax will at least be almost the same in PyO3 0.14.Regarding #694 - the vision was that rather than remove
#[pyfn]
, we would first create a new#[pymodule]
syntax which is used onmod my_module
instead offn my_module
. Then we could deprecate the#[pymodule]
function syntax and#[pyfn]
together.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 would probably avoid this indirection and just add the classes and functions directly:
I guess this makes more sense if you have a big hierarchy of modules with register functions that each import other registers? But at that point you should probably just organize them in submodules.
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 remember now why I originally came up with this pattern of passing
&PyModule
around:Is that expected? Any known workarounds other than what I'm suggesting?
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.
Hm my next idea didn't work either to leverage
use
: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.
Yeah, you're running into #1709
For now you can work around this by writing
use osutil::*;
(As stated in the issue above, I hope we can fix it in a nicer way at the end of the year when we do a dependency update release.)