marmoute added inline comments. INLINE COMMENTS
> martinvonz wrote in nodemap.py:47-49 > nit: call it `serialize()` to match the description? or maybe > `serialize_index()` and rename `_dump_trie()` to `_serialize_trie()` I don't really like `serialize` because we won't have a `deserialize` step. The data should be usable directly with a mmap from disk. So if we standardize function name on something I would rather go toward "persist" (and "parse" for the python function that checks it). This can apply to the "dump" function and docstring. > martinvonz wrote in nodemap.py:57 > nit: replace "rev-0" by "rev 0" so it's not interpreted as subtraction subtraction by zero would be odd, but why not. > martinvonz wrote in nodemap.py:66 > nit: maybe "... involution (is its own inverse)" because i don't think most > people know what an involution is > > Also, you could rename the function to `encode_rev()`. You could then do: > > # encode_rev is its own inverse > decode_rev = encode_rev > > Or not even define that since it's not used I am not convinced by the encode_rev/decode_rev form. I don't think it very important to define `involution`, people can look for it if they need to. The reste of the docstring already explain the functionworks both way. > martinvonz wrote in nodemap.py:85 > nit: will `rev, entry in enumerate(index)` help? (i know we don't care about > speed) We need to index the entry anyway, so that won't change the line much. I find the current form clearer. > martinvonz wrote in nodemap.py:112-118 > The test case still passes with that change, though.... With more eyes, I am seeing what you are doing now. Yeah I think if would work. And it is more consistent with the recursivity used int he previous conditional block. Updating the patch, > martinvonz wrote in nodemap.py:121 > as mentioned above, i think `_serialize_tree()` would be a clearer name (it > matches how you described it in the docstring) I would rather move to `_persist_trie`. What do you think? > martinvonz wrote in nodemap.py:145 > and this could be `_serialize_block()` so it would be `_persist_block()` > martinvonz wrote in nodemap.py:155 > `serialize_value()`? and `_persist_value()` REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7834/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7834 To: marmoute, #hg-reviewers, martinvonz Cc: martinvonz, mjpieters, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
