This revision was automatically updated to reflect the committed changes.
Closed by commit rHGb31a41f24864: rust-cpython: binding for LazyAncestors
(authored by gracinet, committed by ).
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D5441?vs=13007&id=13014
gracinet added a comment.
@yuja this new version exports the LazyAncestors name to Python, expecting
`revlog.ancestors` to do the right thing (as a factory then). I agree that the
destiny of the older `rustlazyancestors` is to disappear, but I'd prefer to
wait a bit before doing that
@kevi
gracinet updated this revision to Diff 13007.
gracinet marked an inline comment as done.
gracinet edited the summary of this revision.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D5441?vs=12952&id=13007
REVISION DETAIL
https://phab.mercurial-scm.org/D5
kevincox added a comment.
In https://phab.mercurial-scm.org/D5441#81222, @yuja wrote:
> Anyway, I just thought using CamelCase in Rust would be slightly nicer and
> simpler. If that makes things complicated, I'll drop my idea and stick to
> anything that is simpler.
I think u
yuja added a comment.
> > Does the class name actually matter? Personally I don't care if
> > lazyancestors() function returns a LazyAncestors object. We'll anyway
> > need a wrapper function to make pure ancestors and rustext ancestors
> > compatible.
>
> Yes, that's
> > Does the class name actually matter? Personally I don't care if
> > lazyancestors() function returns a LazyAncestors object. We'll anyway
> > need a wrapper function to make pure ancestors and rustext ancestors
> > compatible.
>
> Yes, that's in line with your other comments, wh
gracinet marked 2 inline comments as done.
gracinet added a comment.
> Does the class name actually matter? Personally I don't care if
> lazyancestors() function returns a LazyAncestors object. We'll anyway
> need a wrapper function to make pure ancestors and rustext ancestors
> compat
yuja added a comment.
> +/// The purpose of this module is to hide identifiers from other Rust users
> +///
> +/// Some of the identifiers we are defining are meant for consumption
> +/// from Python with other naming conventions. For instance, `py_class!`
> +/// does not let us mak
> +/// The purpose of this module is to hide identifiers from other Rust users
> +///
> +/// Some of the identifiers we are defining are meant for consumption
> +/// from Python with other naming conventions. For instance, `py_class!`
> +/// does not let us make a distinction between the Python and
gracinet added inline comments.
INLINE COMMENTS
> kevincox wrote in ancestors.rs:98
> It should be fine with tbe GIL. RefCell is basically a single-thread lock. In
> this case it should be fine to have no renterance.
Yes, I'll remove these comments, same with the ones for RW lock, it's pretty
kevincox accepted this revision.
kevincox added inline comments.
INLINE COMMENTS
> ancestors.rs:108
> +Err(e) => {
> +return Err(GraphError::new(py, format!("{:?}", e)));
> +}
.map_err(|e| GraphError::new(py, format!("{:?}", e)))
Or even bette
gracinet updated this revision to Diff 12952.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D5441?vs=12878&id=12952
REVISION DETAIL
https://phab.mercurial-scm.org/D5441
AFFECTED FILES
rust/hg-cpython/src/ancestors.rs
tests/test-rust-ancestor.py
CHA
yuja added a comment.
> m.add_class::(py)?;
>
> +m.add_class::(py)?;
Nit: While it's correct per our naming convention, I prefer calling it
as `LazyAncestors` in Rust, and export as `lazyancestors`.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/
> m.add_class::(py)?;
> +m.add_class::(py)?;
Nit: While it's correct per our naming convention, I prefer calling it
as `LazyAncestors` in Rust, and export as `lazyancestors`.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
htt
kevincox accepted this revision.
kevincox added inline comments.
INLINE COMMENTS
> ancestors.rs:98
> +// TODO borrow() can panic, replace with try_borrow()
> +// or consider it's ok thanks to the GIL
> +AncestorsIterator::from_inner(py,
It should be fine with tbe GIL. Ref
gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
For consistency with Python implementation, we're exposing it with the lower
case spelling, so that if one day the whole ancestor module has a R
16 matches
Mail list logo
