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

json lazy parser unparser and support for reading NULL values. #1978

Merged
merged 50 commits into from
Dec 18, 2024

Conversation

jurgenvinju
Copy link
Member

@jurgenvinju jurgenvinju commented Jun 17, 2024

  • read and write JSON null values, every type can register its preferred null representation in Rascal. Maybe[&T] is also builtin supported
  • read and write JSON str values as algebraic data-typed, by providing your own parsers/unparsers for specific data types
  • accurate line/colum, offset/lengths for parse and validation errors
  • tests for the above
  • application of the above to the cytoscape.js bridge to improve the Rascal import graph visualization
  • fix Json random test failed #2098

@jurgenvinju jurgenvinju requested a review from tvdstorm June 17, 2024 15:54
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 38.92473% with 284 lines in your changes missing coverage. Please review.

Project coverage is 49%. Comparing base (043b663) to head (9380bf8).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
...pl/library/lang/json/internal/JsonValueReader.java 35% 230 Missing and 36 partials ⚠️
...pl/library/lang/json/internal/JsonValueWriter.java 60% 4 Missing and 2 partials ⚠️
src/org/rascalmpl/library/lang/json/IO.java 76% 2 Missing and 1 partial ⚠️
src/org/rascalmpl/values/maybe/UtilMaybe.java 76% 3 Missing ⚠️
src/org/rascalmpl/library/util/TermREPL.java 0% 2 Missing ⚠️
src/org/rascalmpl/library/util/Webserver.java 0% 2 Missing ⚠️
src/org/rascalmpl/repl/REPLContentServer.java 0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##              main   #1978    +/-   ##
========================================
  Coverage       49%     49%            
- Complexity    6317    6346    +29     
========================================
  Files          666     667     +1     
  Lines        59695   59836   +141     
  Branches      8670    8691    +21     
========================================
+ Hits         29600   29725   +125     
+ Misses       27863   27857     -6     
- Partials      2232    2254    +22     

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

@jurgenvinju jurgenvinju self-assigned this Jun 17, 2024
@jurgenvinju jurgenvinju marked this pull request as ready for review June 17, 2024 20:17
@jurgenvinju
Copy link
Member Author

@tvdstorm I added the null values here. Do you fancy the solution?

@jurgenvinju
Copy link
Member Author

Since this adds parameters to Java methods we have to wait until after the current release cycle to merge this.

@jurgenvinju jurgenvinju changed the title json lazy parser unparser json lazy parser unparser and support for reading NULL values. Jun 26, 2024
@DavyLandman
Copy link
Member

DavyLandman commented Jul 4, 2024

A lot of things are happening in this PR, and I'm missing:

Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

The changes to JsonValueReader are hard to oversee, it mostly looks fine, but I would want to take some time and test the impact on VS Code and it's use of the JsonValueReader class.

pom.xml Outdated
@@ -164,7 +164,7 @@
<configuration>
<isPackageCourse>false</isPackageCourse>
<enableStandardLibrary>false</enableStandardLibrary>
<errorsAsWarnings>false</errorsAsWarnings>
<errorsAsWarnings>true</errorsAsWarnings>
Copy link
Member

Choose a reason for hiding this comment

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

do we want this as part of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary for bootstrapping the tutor

Copy link
Member Author

Choose a reason for hiding this comment

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

still going to see if we can minimize the number of failing pages before merging.

@jurgenvinju
Copy link
Member Author

The changes to JsonValueReader are hard to oversee, it mostly looks fine, but I would want to take some time and test the impact on VS Code and it's use of the JsonValueReader class.

The changes have been designed such that there can't be impact on existing uses of the JSON IO clients, unless they were waiting for NPEs for inputs with null inside. Those exceptions will not be thrown anymore and values will be constructed to represent those null values.

Recommend to focus on that distintion. Typically catch( NullpointerException e) in java or catch Java("NullpointerException") in Rascal.

@jurgenvinju
Copy link
Member Author

The other changes are only activated by providing new actual keyword parameters to format datatypes to string or parse strings to datatypes. If not provided the code behaves as before.

@DavyLandman
Copy link
Member

Is there a way to increase the test coverage? Only 35% of the added lines for the JSONReader are covered according to codecov.

@jurgenvinju
Copy link
Member Author

jurgenvinju commented Dec 18, 2024

Is there a way to increase the test coverage? Only 35% of the added lines for the JSONReader are covered according to codecov.

The reason is that the IO tests use the @deprecated JSON readers and writers. To change this we need to simply replace the reader by the new parser and the writer by the new unparser in the readWriteTest, but the new issues that come up are both very old and pretty diverse. I want to capture the fixes in a new PR.

"new" here is already 8 years (!!!), btw. It's really time to remove the deprecated functions and continue with the new ones

@jurgenvinju jurgenvinju merged commit d0cce09 into main Dec 18, 2024
6 of 7 checks passed
@jurgenvinju jurgenvinju deleted the json-lazy-parser-unparser branch December 18, 2024 09:05
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.

Json random test failed
2 participants