search

xmtp / didethresolver

XMTP Registry Resolver
MIT License
3 stars 1 forks source link

34: replace url crate with peg parser based on RFC 3986 #39

Closed jac18281828 closed 8 months ago

jac18281828 commented 8 months ago

closes #34

general cleanup, make DidUrl immutable by removing setter

fix rustdoc

codecov[bot] commented 8 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a30b4da) 95.81% compared to head (64b6ca7) 96.67%.

Files Patch % Lines
lib/src/types/ethr.rs 89.28% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #39 +/- ## ========================================== + Coverage 95.81% 96.67% +0.85% ========================================== Files 13 13 Lines 2153 2704 +551 ========================================== + Hits 2063 2614 +551 Misses 90 90 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

insipx commented 8 months ago

🧹 url 🧹

nice! I like the contextual errors for parsing, I hope it will make it easier on users if they get the DID wrong

jac18281828 commented 8 months ago

Would it make sense to use map instead of an array for queries? It's more efficient to search keys. Also, would it make sense to test immutability? i.e. checking that original object is not modified after calling with_[query/path/attribute/...]?

It's a good idea to use the map but it will not work because the names and values are not distinct in the specification. A query could have a=a, a=a, a=a and be valid syntax its application dependent what the meaning of that is. Imagine the protocol is 'execute this instruction'.

The performance of an array vs a map for 2-3 elements is probably actually better for lookup in a typical application where every parameter will have meaning. Since we are using 1 at the moment we can probably ignore this issue.

The test of immutability should be guaranteed by rust ownership rules, but I will add a check for sanity.

37ng commented 8 months ago

The test of immutability should be guaranteed by rust ownership rules, but I will add a check for sanity.

In unit tests, imo it's crucial not to depend on implementation specifics. For functions with_*** functions, they should ideally return a new object rather than altering the original one. Our tests must verify this behavior directly, avoiding assumptions about Rust's clone() method or omitting such tests.

Agreed with the query data structure, thanks for explaining.

jac18281828 commented 8 months ago

Please take a last look. I believe it's ready to go. I updated with some cleanup: