- User Since
- Jan 11 2018, 4:59 AM (67 w, 10 h)
Thanks for the changes.
Sat, Apr 20
Wed, Apr 17
Mon, Apr 15
Sun, Apr 14
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 the type system to ensure that you are using the class correctly. However it appears that this is just re-implementing an API so improving that API is probably out of scope for the time being.
Feb 16 2019
Feb 12 2019
Jan 15 2019
Jan 14 2019
Jan 12 2019
Jan 3 2019
Dec 22 2018
Dec 15 2018
Dec 12 2018
This is a nice improvement. It still seems odd to me to return a list of two parents when there could be 0, 1 or 2 parents. I guess this is for performance reasons?
Dec 6 2018
I think sticking close to the python version makes sense for the initial version. Then improvements can be made in follow-ups.
Dec 4 2018
Overall looks good to me. Just a couple of points.
May 6 2018
Mar 24 2018
The latest changes are looking really good. I have a couple more comments but I didn't have time for a full review. I'll try to get more reviewed tomorrow. It seems that you still have a lot of stuff still in-flight so I'll try to slowly review the changes as I have time. If you want input/feedback on any particular part just ask and I will prioritize it.
Mar 21 2018
Ah, I forgot to consider the python interop. Now the need for that crate makes sense. Thanks for explaining.
Mar 20 2018
I'm not a windows expert but it seems like the rust OsStr, Path and filesystem APIs should handle these conversions for you. I think the only place where you would need to do os-specific code is when doing serialization and serialization which I think should be handled by https://doc.rust-lang.org/std/os/unix/ffi/trait.OsStringExt.html and https://doc.rust-lang.org/std/os/windows/ffi/trait.OsStringExt.html.
Mar 8 2018
Rust has platform independent types PathBuf and &Path for paths and OsString and &OsStr for strings (owned and references respectively. They do have os-specific extensions but as long as you don't use them it should be cross platform. That being said, if you are serializing and deserializing them you may need to write some platform dependant code.
I will try to finish the review later, but it might be quicker if you incorporate some of the changes first since a lot of them are repeated many times. Overall it looks good, there are a couple of things that i would highlight to make the code easier to read.
Feb 7 2018
I agree with the splitting comments :) In fact there might already be a base85 crate which can be used: https://docs.rs/zero85. Either way I'll hold off on the review, feel free to ping me when you are ready for me to take a look.
Jan 11 2018
Overall it looks great. I added a bunch of nitpicks. The most common suggestion was to add some contextual information to error messages. Other then that mostly minor style improvements. Feel free to push back on anything you don't agree with :)