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

expanded context implementation #31

Merged
merged 7 commits into from
Sep 12, 2023

Conversation

hugoib
Copy link
Collaborator

@hugoib hugoib commented Sep 7, 2023

Tests are running, now the context implementation supports an object.

Copy link
Collaborator

@IngridPuppet IngridPuppet left a comment

Choose a reason for hiding this comment

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

You mention some tests in the PR description. It would be nice to have them as unit tests in the codebase. You have certainly noticed how many of such are stacked at the end of this file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you give a chance to the type serde_json::Value::Object and compare how it fits?


#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
pub enum JsonItem {
SingleString(String),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like with this, the whole JsonObject could resolve to a single String, which is probably not the intent.

@IngridPuppet IngridPuppet linked an issue Sep 7, 2023 that may be closed by this pull request
@hugoib
Copy link
Collaborator Author

hugoib commented Sep 11, 2023

Since MR #32 changes how context is being used in the project, I brought the changes from that MR and then used serde_json::Value::Object to expand that implementation

Merge #32 first.

Closes #32

@IngridPuppet IngridPuppet merged commit ad2a419 into main Sep 12, 2023
1 check passed
@hugoib hugoib deleted the iss-23-modify-context-implementation branch September 14, 2023 13:45
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.

Modify context implementation in did-utils to also contain objects
3 participants