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

fix: minor type hint corrections for VcLdpManager #2704

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Jan 10, 2024

These are some really minor fixes just to make type checking happy and to clarify the use of some values in the VcLdpManager.

@dbluhm dbluhm force-pushed the fix/vc-ldp-manager-types branch from 57f1b84 to ad0e60d Compare January 10, 2024 13:18
@dbluhm dbluhm requested review from PatStLouis and swcurran January 10, 2024 13:56
suites.append(
suite(
return [
cast(
Copy link
Contributor

Choose a reason for hiding this comment

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

This section could use a short comment to understand what is happening, there's a lot going on in the return statement.

I could suggest:
# We iterate through the PROOF_KEY_TYPE_MAPPING dictionary to return an array of it's items as LinkedDataProof objects

@PatStLouis
Copy link
Contributor

Looks good, I see a lot of attributes are marked as Optional, if they were required beforehand, what is the purpose of making them optional?

Otherwise I just want to point out that technically the term LinkedDataProof was renamed to DataIntegrityProof. Given the structure of the project this would be a pretty big change to implement however it's important to note that when we are referring to linked data proofs here we are in fact talking about data integrity proofs...maybe a note somewhere in the readme could be worthwhile...
https://w3c.github.io/vc-data-integrity/
https://www.w3.org/TR/vc-data-model/#data-integrity-proofs

@dbluhm
Copy link
Contributor Author

dbluhm commented Jan 10, 2024

Looks good, I see a lot of attributes are marked as Optional, if they were required beforehand, what is the purpose of making them optional?

Good question; the type hints for the changed parameters all had a default value of None. This was intended to indicate that the value was a str | None or whatever other type. This was an old convention that ACA-Py followed but is no longer accepted by tools like pyright/pylance. The added Optional annotation aligns the type hint with the use of the field, updating it to match what modern type checking tools expect.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jan 10, 2024

Otherwise I just want to point out that technically the term LinkedDataProof was renamed to DataIntegrityProof. Given the structure of the project this would be a pretty big change to implement however it's important to note that when we are referring to linked data proofs here we are in fact talking about data integrity proofs.

I think this is a good one for us to file an issue on but probably not something to be addressed by this PR directly, IMO.

@PatStLouis
Copy link
Contributor

I definitely don't think this PR should address this as this would entail a massive restructuring and might become a significant endeavor. Deciding if it's a worthwhile thing to address could be discussed in a separate issue. There's a distinction here between functional coding and proper term definition. AFAIK linked data proofs and data integrity proofs refer to the same thing/concept.

proof: dict = None,
verification_method: str = None,
proof: Optional[dict] = None,
verification_method: Optional[str] = None,
date: Union[datetime, None] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
date: Union[datetime, None] = None,
date: Optional[datetime] = None,

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@swcurran swcurran merged commit e393804 into openwallet-foundation:main Jan 16, 2024
8 checks passed
@dbluhm dbluhm deleted the fix/vc-ldp-manager-types branch January 30, 2024 21:30
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.

4 participants