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 json #70

Merged
merged 4 commits into from
Sep 6, 2023
Merged

Fix json #70

merged 4 commits into from
Sep 6, 2023

Conversation

RobbinBouwmeester
Copy link
Contributor

Fairly straightforward fix of writing valid JSON.

Downstream testing after accepting PR and merging this into Proteobench:

Proteobot/Results_quant_ion_DDA@3df7832

Also see: #65

Fix json so it is valid (spreads over multiple lines with records).
Copy link
Member

@enryH enryH left a comment

Choose a reason for hiding this comment

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

I hope the json was valid before, but I also prefer this styling. I guess the one line reprentation is more efficient, but this is definitely easier to read!

Copy link
Contributor

@vedran-kasalica vedran-kasalica left a comment

Choose a reason for hiding this comment

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

I agree with the changes, they solve Issue #65. I would recommend using the same on line 23 of the same file (as part of the write_json_local_development method).

@RobbinBouwmeester RobbinBouwmeester marked this pull request as ready for review September 6, 2023 13:55
@RobbinBouwmeester RobbinBouwmeester merged commit 5a45b64 into main Sep 6, 2023
4 checks passed
@RobbinBouwmeester RobbinBouwmeester deleted the fix_json branch September 6, 2023 14:00
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