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

IsValid should take in a context and return err #395

Closed
andresuribe87 opened this issue May 24, 2023 · 5 comments
Closed

IsValid should take in a context and return err #395

andresuribe87 opened this issue May 24, 2023 · 5 comments
Labels
dids enhancement New feature or request good first issue Good for newcomers

Comments

@andresuribe87
Copy link
Contributor

The function below is hiding information from the users of the SDK, so it's difficult to understand what failed. The return type should be error.

Additionally, it should take in a context.Context as it's first parameter because it's doing a network lookup.

It's debatable whether the name of the function should stay as IsValid. If returning an error, consider naming Validate.

func (d DIDWeb) IsValid() bool {

@andresuribe87 andresuribe87 added enhancement New feature or request good first issue Good for newcomers dids labels May 24, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in SSI May 24, 2023
@decentralgabe decentralgabe added this to the Milestone v0.0 milestone May 24, 2023
@decentralgabe decentralgabe moved this from 🆕 New to 📋 Backlog in SSI May 24, 2023
@vlad-tim
Copy link
Contributor

I have not been able to find usages of the function that have to be adjusted as the function signature changes, can you advise?

@andresuribe87
Copy link
Contributor Author

Thanks for taking a look @vlad-timofeev !

There are no internal uses of this function. The SSI-service consumes this lib, so when it's updated, we'll need to make sure to also update the callers in there.

For this issue I would recommend adding a unit test so we know this works as expected.

@vlad-tim
Copy link
Contributor

Thank you for clarifying @andresuribe87 !

Created #399

@vlad-tim
Copy link
Contributor

vlad-tim commented Jun 7, 2023

@andresuribe87 is there anything else that should be updated to account for changes in #399 ?

@andresuribe87
Copy link
Contributor Author

There isn't, thanks @vlad-timofeev !

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in SSI Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dids enhancement New feature or request good first issue Good for newcomers
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants