This revision was automatically updated to reflect the committed changes.
Closed by commit rHG10b465d61556: rust-discovery: starting core implementation
(authored by gracinet, committed by ).
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
kevincox added a comment.
In https://phab.mercurial-scm.org/D6231#91102, @gracinet wrote:
> I believe to have addressed your immediate concerns.
>
> I played with splitting in different `struct`s for the various stages of
the process. It is indeed cleaner and clearer Rust code.
>
gracinet marked an inline comment as done.
gracinet added a comment.
I believe to have addressed your immediate concerns.
I played with splitting in different `struct`s for the various stages of the
process. It is indeed cleaner and clearer Rust code.
However mutating in place of
gracinet updated this revision to Diff 14794.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D6231?vs=14744=14794
REVISION DETAIL
https://phab.mercurial-scm.org/D6231
AFFECTED FILES
rust/hg-core/src/discovery.rs
rust/hg-core/src/lib.rs
CHANGE
kevincox added inline comments.
INLINE COMMENTS
> gracinet wrote in discovery.rs:93
> I'm not sure. It wouldn't be a straight error to call this early, as long as
> the caller knows what this means, but I don't have any use-case before the
> discovery is complete.
>
> On the other hand, I
gracinet added inline comments.
INLINE COMMENTS
> kevincox wrote in discovery.rs:57
> You can replace the early return by an `if let`. This also removed the
> `.unwrap()` which is always nice.
>
> if let Some(ref mut undecided) = self.undecided {
>
gracinet updated this revision to Diff 14744.
gracinet marked 2 inline comments as done.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D6231?vs=14720=14744
REVISION DETAIL
https://phab.mercurial-scm.org/D6231
AFFECTED FILES
kevincox added a comment.
In https://phab.mercurial-scm.org/D6231#90756, @gracinet wrote:
> Yes, that's what I had in mind. Not sure where that enum would be defined
(hg-cpython or hg-core) but it doesn't matter so much.
I would probably stick it in hg-cpython because it is a
gracinet added a comment.
> What I would probably do is still implement the Rust part with multiple
structs but then wrap it in an enum for the python API.
Yes, that's what I had in mind. Not sure where that enum would be defined
(hg-cpython or hg-core) but it doesn't matter so much.
kevincox added a comment.
In https://phab.mercurial-scm.org/D6231#90744, @gracinet wrote:
> That's an interesting idea. I expect it to solve my gripes with
`ensure_undecided()` neatly. I'm not sure how it'd fare on the Python side, and
probably wouldn't do it in the bindings to maintain
gracinet added a comment.
@kevincox Thanks for the reviews !
> It would probably be better to have 3 classes and when you apply new
information each class transitions into the next one. This allows the type
system to ensure that you are using the class correctly.
That's an
kevincox accepted this revision.
kevincox added a comment.
I'm not a huge fan of the design of this class because it has a strong
protocol to use correctly. It would probably be better to have 3 classes and
when you apply new information each class transitions into the next one. This
allows
gracinet created this revision.
Herald added subscribers: mercurial-devel, mjpieters, kevincox, durin42.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
Once exposed to the Python side, this core object will avoid
costly roundtrips with potentially big sets of revisions.
This
13 matches
Mail list logo
