Skip to content
This repository was archived by the owner on Feb 28, 2025. It is now read-only.

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

Merged
merged 3 commits into from
Feb 3, 2024

Conversation

jac18281828
Copy link
Contributor

closes #34

  • adding path + query + fragment
  • test coveragee
  • parses query as a vec of string tuple

general cleanup, make DidUrl immutable by removing setter

fix rustdoc

- adding path + query + fragment
- test coveragee
- parses query as a vec of string tuple

general cleanup, make DidUrl immutable by removing setter

fix rustdoc
@jac18281828 jac18281828 added the enhancement New feature or request label Feb 1, 2024
@jac18281828 jac18281828 self-assigned this Feb 1, 2024
Copy link

codecov bot commented Feb 1, 2024

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 ⚠️
Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@insipx
Copy link
Contributor

insipx commented Feb 1, 2024

🧹 url 🧹

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

Copy link
Contributor

@37ng 37ng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/...]?

@jac18281828
Copy link
Contributor Author

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
Copy link
Contributor

37ng commented Feb 2, 2024

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
Copy link
Contributor Author

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

  • removed tiny-keccak
  • added parsing support with a test for public key did....I suspect pks are still not working and we need to add a ticket to test that and verify it is all good.
  • cleaned up account creation
  • added coverage for codecov complaints

@jac18281828 jac18281828 force-pushed the jac/scanner branch 2 times, most recently from b316cdd to 3caa186 Compare February 3, 2024 01:08
- code is more rust-like using From and into
@jac18281828 jac18281828 merged commit 64b6ca7 into main Feb 3, 2024
@jac18281828 jac18281828 deleted the jac/scanner branch February 3, 2024 01:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

rewrite ethr parser
3 participants