Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ServiceBuilder #1021

Merged
merged 4 commits into from
Oct 26, 2023
Merged

Refactor ServiceBuilder #1021

merged 4 commits into from
Oct 26, 2023

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Oct 21, 2023

Builder refactoring

  • Remove extra alias indirection ServiceTypeAlias - all its usage (2 places) fits on one screen

  • Remove add_service_types in favor of add_service_type. The removed function was only used once and easily replacable by its "singular" version

  • Remove ServiceBuilderWithServiceType - ServiceType is in fact optional, as https://sovrin-foundation.github.io/sovrin/spec/did-method-spec-template.html says:

    If the types field is present

    So requiring user to call one of the (originally 2 methods) is just not right. If we were to be just "opinionated" to require users of our builder to do so, it should follow same pattern as other required attributes such as id, service_endpoint, extra.

  • If add_service_type is called with the same value as been provided before, throw InvalidInput instead of quietly doing nothing

  • If add_service_type is called with empty string, throw InvalidInput error

Note:
Probably stating obvious, but I take it that ServiceBuilder was somewhat of a shortcut as we didn't intend to use builder macro libraries at the time, and building full blown state pattern builder by hand is cumbersome (hence why required attributes are part of ServiceBuilder<E>::new(....) constructor directly. Perhaps down the line we can adopt the same builder library as in messages crate.

Additional

Squashed some additional tweak, don't want create crate X pull requests so I'll squash further smaller improvements here.

  • Implement Debug for Didso it prints Did { did: "did:foo:bar", method: Some("foo"), id: "bar" } instead of previously Did { did: "did:foo:bar", method: Some(4..7), id: 8..11 }
  • Implement Display for DidDocument which prints the ddo as json

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2023

Codecov Report

Merging #1021 (0b2e21e) into main (1a414f0) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1021      +/-   ##
==========================================
- Coverage   36.31%   36.30%   -0.01%     
==========================================
  Files         386      386              
  Lines       22040    22044       +4     
  Branches     4062     4062              
==========================================
  Hits         8003     8003              
- Misses      11877    11881       +4     
  Partials     2160     2160              
Flag Coverage Δ
unittests-aries-vcx 36.30% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
did_parser/src/did.rs 0.00% <0.00%> (ø)

@Patrik-Stas Patrik-Stas force-pushed the refactor/service-builder branch 2 times, most recently from f7b039e to 37a2d0c Compare October 22, 2023 20:55
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas merged commit 47f9c0d into main Oct 26, 2023
29 checks passed
@Patrik-Stas Patrik-Stas deleted the refactor/service-builder branch October 26, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants