This is an archive of the discontinued Mercurial Phabricator instance.

rust-cpython: binding for AncestorsIterator
ClosedPublic

Authored by gracinet on Dec 15 2018, 6:42 AM.

Details

Summary

It's now reachable from Python as
rustext.ancestor.AncestorsIterator

Tests are provided in the previously introduced
Python testcase: this is much more convenient
that writing lengthy Rust code to call into Python.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gracinet created this revision.Dec 15 2018, 6:42 AM
kevincox accepted this revision.Dec 15 2018, 7:59 AM
kevincox added inline comments.
rust/hg-cpython/src/ancestors.rs
33
revs.len(py).unwrap_or(0)
41
result_revpy?.extract(py)?
yuja added a subscriber: yuja.Dec 15 2018, 11:02 PM

// GNU General Public License version 2 or any later version.

  • //! Bindings for the hg::ancestors module provided by the

Nit: perhaps the empty line was removed by mistake.

+fn reviter_to_revvec(py: Python, revs: PyObject) -> PyResult<Vec<Revision>> {
+ let revs_iter = revs.iter(py)?;
+ we need to convert to a vector, because Python iterables can
+
raise errors at each step.
+ let cap = match revs.len(py) {
+ Ok(l) => l,
+ Err(_) => 0, // unknown
+ };
+ let mut initvec: Vec<Revision> = Vec::with_capacity(cap);
+ for result_revpy in revs_iter {
+ let as_pyint: PyInt = match result_revpy {
+ Err(e) => {
+ return Err(e);
+ }
+ Ok(revpy) => revpy.extract(py)?,
+ };
+ initvec.push(as_pyint.value(py) as Revision);
+ }
+ Ok(initvec)

revs_iter.map(|r| r.and_then(|o| o.extract::<Revision>(py))).collect()

PyInt::value() isn't supported on Python 3, and it should be better to
extract int directly with bounds checking.

https://dgrunwald.github.io/rust-cpython/doc/cpython/struct.PyInt.html#method.value

+impl AncestorsIterator {
+ pub fn from_inner(py: Python, ait: CoreIterator<Index>) -> PyResult<Self> {

Nit: I slightly prefer spelling it as hg::AncestorsIterator.

@yuja thanks for spotting the Python3 incompatibility. As you can guess, I didn't compile with Python 3, and actually I hadn't even defined the features to that extent.
I will submit separately a change that takes care of the build with Python 3 before updating that one (but yes, got test-rust-ancestor.py to pass).
OTOH I'll have to keep the loop so that I can get the early return upon error (otherwise we'd get a Vec<PyResult<...>>

@kevincox thanks for the shorter version, wasn't used to these sugars when I wrote this long ago :-)

yuja added a comment.Dec 17 2018, 9:33 AM
OTOH I'll have to keep the loop so that I can get the early return upon error (otherwise we'd get a Vec<PyResult<...>>

I didn't check the implementation, but collect() for Result will return
immediately if reached to Err.

@yuja you're right! My first attempt was tainted with assigning to a Vec, whereas upon type inference with the actual return type PyResult<Vec>, the compiler does the right thing so that was the escape plan for early returns in closures :-) amazing.

gracinet updated this revision to Diff 12950.Dec 22 2018, 11:23 AM
gracinet updated this revision to Diff 12953.

@yuja actually my first version of CoreIterator (and later CoreLazy) used the hg::AncestorsIterator spelling, but actually I believe we might end up after a while not exporting everything at the top of the hg crate, leaving us either to use hg::ancestors, giving us a long and puzzling ancestors::AncestorsIterator or directly as hg:ancestors::AncestorsIterator. I don't think it would be tasteful to use hg::ancestors as core.

That's why I found it clearer to call them CoreIterator etc. But I can change it if you really dislike them.

This revision was automatically updated to reflect the committed changes.