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

Write migration guide #795

Merged
merged 7 commits into from
Mar 13, 2020
Merged

Write migration guide #795

merged 7 commits into from
Mar 13, 2020

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Mar 7, 2020

Resolves #785
I wrote a migration guide in Appendix B of the guide.
Now only pushed the migration guide, but I'm going to use this branch for the release.

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.

A migration guide is a really good idea. This is really well written and has lots of good examples!

I made some suggestions to small bits of the wording.

guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member Author

kngwyu commented Mar 8, 2020

@davidhewitt
Thank you for the reviews!

@@ -42,10 +42,11 @@ For more, see [the constructor section](https://pyo3.rs/master/class.html#constr
PyO3 0.9 introduces [`PyCell`](https://pyo3.rs/master/doc/pyo3/pycell/struct.PyCell.html), which is
a [`RefCell`](https://doc.rust-lang.org/std/cell/struct.RefCell.html) like object wrapper
for dynamically ensuring
[Rust's rule of Reference](https://doc.rust-lang.org/book/ch04-02-references-and-borrowing.html#the-rules-of-references).
[Rust's rules of references](https://doc.rust-lang.org/book/ch04-02-references-and-borrowing.html#the-rules-of-references).
Copy link
Member

Choose a reason for hiding this comment

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

I would be more explicit and say "rules regarding aliasing of references".

Copy link
Member Author

@kngwyu kngwyu Mar 9, 2020

Choose a reason for hiding this comment

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

Yeah, but I want to use the same expression as the rustbook here

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I just don't think "the rules of references" is a standing term in the community. I wouldn't immediately know what it refers to until clicking the link.

Copy link
Member

Choose a reason for hiding this comment

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

Why not have both and write something like

RefCell-like object wrapper for ensuring Rust's rules regarding aliasing of references are upheld.
For more detail, see the 
[Rust Book's section on Rust's rules of references](https://doc.rust-lang.org/book/ch04-02-references-and-borrowing.html#the-rules-of-references)

guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
guide/src/migration.md Outdated Show resolved Hide resolved
@kngwyu kngwyu changed the title Write migration guide and relase 0.9.0 Write migration guide and release 0.9.0 Mar 9, 2020
kngwyu and others added 2 commits March 9, 2020 14:11
Co-Authored-By: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@kngwyu kngwyu marked this pull request as ready for review March 9, 2020 05:40
@kngwyu
Copy link
Member Author

kngwyu commented Mar 9, 2020

Pushed some changes for rust-numpy.


### PyCell
PyO3 0.9 introduces [`PyCell`](https://pyo3.rs/master/doc/pyo3/pycell/struct.PyCell.html), which is
a [`RefCell`](https://doc.rust-lang.org/std/cell/struct.RefCell.html) like object wrapper
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
a [`RefCell`](https://doc.rust-lang.org/std/cell/struct.RefCell.html) like object wrapper
a [`RefCell`](https://doc.rust-lang.org/std/cell/struct.RefCell.html)-like object wrapper

fn as_ref(&self, _py: Python) -> &PyCell<T> {
impl<T> AsPyRef for Py<T>
where
T: PyTypeInfo,
Copy link
Member

Choose a reason for hiding this comment

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

👍

Cargo.toml Outdated
num-bigint = { version = ">= 0.2", optional = true }
num-complex = { version = ">= 0.2", optional = true }
num-traits = "0.2.8"
indoc = "^0.3.4"
Copy link
Member

Choose a reason for hiding this comment

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

^ is the default operator, this doesn't change anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I misunderstood that it is equal to "=="

@kngwyu kngwyu changed the title Write migration guide and release 0.9.0 Write migration guide Mar 13, 2020
@kngwyu
Copy link
Member Author

kngwyu commented Mar 13, 2020

I decided to merge this PR now and will open a separate PR for the release for other PRs to sync with the master easier.

@kngwyu kngwyu merged commit 9c3331e into master Mar 13, 2020
@kngwyu kngwyu deleted the relase-0.9 branch March 13, 2020 10:12
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.

Porting to 0.9 after #770
3 participants