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

chore(fedimint-cli): convert invalid JSON data to JSON string #4088

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

15IITian
Copy link
Contributor

@15IITian 15IITian commented Jan 22, 2024

If params fails to get parsed into JSON -> then convert it into valid JSON String wrapped in Value::String

fix #3194

@15IITian 15IITian requested a review from a team as a code owner January 22, 2024 07:24
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (116faaf) 58.32% compared to head (8dbb9ad) 58.36%.

Files Patch % Lines
fedimint-cli/src/lib.rs 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4088      +/-   ##
==========================================
+ Coverage   58.32%   58.36%   +0.04%     
==========================================
  Files         192      192              
  Lines       42646    42655       +9     
==========================================
+ Hits        24872    24895      +23     
+ Misses      17774    17760      -14     

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

fedimint-cli/src/lib.rs Outdated Show resolved Hide resolved
@15IITian 15IITian marked this pull request as draft January 22, 2024 13:05
@15IITian 15IITian marked this pull request as ready for review January 22, 2024 16:20
@15IITian 15IITian changed the title chore(fedimint-cli): convert invalid JSON_string to Value(String) chore(fedimint-cli): convert invalid JSON data to JSON string Jan 23, 2024
@15IITian 15IITian changed the title chore(fedimint-cli): convert invalid JSON data to JSON string chore(fedimint-cli): convert invalid JSON data to JSON string Jan 23, 2024
fedimint-cli/src/lib.rs Outdated Show resolved Hide resolved
@GetPsyched
Copy link
Contributor

GetPsyched commented Jan 23, 2024

Please mark each review as resolved after you've added the suggested changes; it clutters the PR view for future reviews and/or reviewers.

fedimint-cli/src/lib.rs Outdated Show resolved Hide resolved
elsirion
elsirion previously approved these changes Jan 24, 2024
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

Looks useful, some nits

fedimint-cli/src/lib.rs Show resolved Hide resolved
fedimint-cli/src/lib.rs Outdated Show resolved Hide resolved
If `params` fails to get parsed into JSON -> then convert it into `JSON String`
Added debug statement to tell that if given params data is not seriazable , then converting it into
JSON string
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

There might be some funny edge cases, like null being valid JSON, but overall this will be a quality of life improvement for this debug tool.

@15IITian
Copy link
Contributor Author

15IITian commented Jan 25, 2024

There might be some funny edge cases, like null being valid JSON

IYDM , Should I solve it in this PR ?

@elsirion
Copy link
Contributor

I don't think it's solvable without creating other edge-cases.

@15IITian
Copy link
Contributor Author

Ok

@dpc dpc added this pull request to the merge queue Jan 26, 2024
Merged via the queue into fedimint:master with commit 08aa4fa Jan 26, 2024
20 checks passed
@15IITian 15IITian deleted the json branch January 31, 2024 03:35
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.

fedimint-cli dev api usability is meh.
5 participants