gracinet added inline comments. gracinet marked 3 inline comments as done. INLINE COMMENTS
> kevincox wrote in nodemap.rs:37 > Can you please add doc-comments for this? I find that documenting trait > methods is especially important. Sure, indeed it's more important than with the `impl`. > martinvonz wrote in nodemap.rs:150 > How about something like this then? > > type BlockSource = Box<dyn Deref<Target = [Block]> + Send>; > type ByteSource = Box<dyn Deref<Target = [u8]> + Send>; > > I won't insist, so up to you if you think it helps. Missed the type (not trait) alias suggestion. Maybe for the next update, then > martinvonz wrote in nodemap.rs:153 > I understand that you picked this interface because it works well for the > caller, but it feel a little weird to always return `None` or `Some(rev)` > where `rev` is one of the function inputs. It's practically a boolean-valued > function. Do the callers get much harder to read if you actually make it > boolean-valued? Perhaps a better name would be better than this `has_` that indeed feels boolean? `check_prefix`? `confirm` ? Previous naming was `validate_candidate`, but that very same name is used at the end of the series when it becomes involved with `NULL_NODE` etc. Then this `has_prefix_or_none` becomes one step in the final verification. Returning a `bool` would mean adding a closure to the callers. making it less obvious that they are just chaining the maturation of the response. I'm leaving unchanged for now, not sure what the best is. Still note that this is a private function, there's no risk an external caller would be confused by what it does. > kevincox wrote in nodemap.rs:205 > I don't think the `<'_>` is necessary. Removed REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7791/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7791 To: gracinet, #hg-reviewers, kevincox Cc: Alphare, martinvonz, durin42, kevincox, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
