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

9314 Get JSON object from JsonUtil #9923

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

bencomp
Copy link
Contributor

@bencomp bencomp commented Sep 15, 2023

What this PR does / why we need it:

Work around a potential resource leak by using the existing JsonUtil (as suggested by @qqmyers)

Which issue(s) this PR closes:

Closes #9314

Special notes for your reviewer:
You're welcome, @pdurbin.

Suggestions on how to test this:
See Sonarcloud's number of issues go down.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No

Is there a release notes update needed for this change?:
No

Additional documentation:
Perhaps add a note to the developer docs to make sure this doesn't happen again?

@coveralls
Copy link

Coverage Status

coverage: 20.045%. remained the same when pulling 2ae1a9f on bencomp:9314-get-json-object into 4e1bc5b on IQSS:develop.

@pdurbin pdurbin added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation Sep 15, 2023
@pdurbin pdurbin moved this from Ready for Review ⏩ to In Review 🔎 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Sep 15, 2023
@pdurbin pdurbin self-assigned this Sep 15, 2023
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

@bencomp thanks! Looks like a good fix to me.

I'll pass this onto QA once the Jenkins tests pass. That said, it probably doesn't matter because I don't see any code coverage for these lines at https://coveralls.io/builds/62689803/source?filename=src%2Fmain%2Fjava%2Fedu%2Fharvard%2Fiq%2Fdataverse%2Futil%2Fjson%2FJsonParser.java#L683

Screen Shot 2023-09-15 at 12 12 55 PM

@kcondon when this comes to you, it looks like we do special handling of invalid country names when parsing our native JSON to create a dataset:

Screen Shot 2023-09-15 at 12 10 18 PM

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Jenkins tests are passing. Approved.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from In Review 🔎 to Ready for QA ⏩ Sep 18, 2023
@pdurbin pdurbin removed their assignment Sep 18, 2023
@kcondon
Copy link
Contributor

kcondon commented Sep 22, 2023

@pdurbin Do you know where this code gets used in the app for regression testing purposes?

@kcondon kcondon self-assigned this Sep 22, 2023
@kcondon kcondon merged commit 5fc7b30 into IQSS:develop Sep 22, 2023
9 checks passed
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Ready for QA ⏩ to Done 🚀 Sep 22, 2023
@pdurbin pdurbin added this to the 6.1 milestone Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
4 participants