Page MenuHomePhabricator

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
Lint Skipped
Unit
Unit Tests Skipped

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.