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

Skip json comments #1448

Merged
merged 5 commits into from
Jun 11, 2022
Merged

Skip json comments #1448

merged 5 commits into from
Jun 11, 2022

Conversation

mlaily
Copy link
Contributor

@mlaily mlaily commented Jun 9, 2022

Closes #1215

JSON.Net supports both // and /* .... */ style comments (https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_Linq_CommentHandling.htm), can FSharp.Data be extended to do the same?

Feel free to discuss!

One thing of note: there is no way to disallow comments in json now.
I didn't feel like it was something the user would usually want, but there might be some edge cases where it could be needed (especially since it's not standard json).
I'm not sure what would be the best way to support this option without littering the parsing methods with a boolean or something like that... (and how to keep backward compat)

Bonus:

  • I fixed the tests (first commit) that were failing. Not sure if this is normal/known?
  • I changed the .gitattributes to format new lines the same way .editorconfig wants them: lf for almost all file types. (I did this after having multiple problems while working in the code and having mixed CRLF and LF...)
    (These are the same changes I did in PR Implement "inline schemas": ability to add type hints into the type providers' source documents #1447. I'm thinking that it might be easier to get them accepted in this small PR, so I can remove them from the big one and make it easier to review)

@cartermp
Copy link
Collaborator

cartermp commented Jun 9, 2022

Test failure on windows:

The 'date' attribute is invalid - The value '2018-08-29T05:30:56.0000000' is invalid according to its datatype 'http://www.w3.org/2001/XMLSchema:date' - The string '2018-08-29T05:30:56.0000000' is not a valid Date value./n<A int="0" long="0" date="2018-08-29T05:30:56.0000000" dateTime="2022-06-09T20:33:59.5191679+00:00" boolean="false" decimal="0" double="NaN" />
Failed!  - Failed:     1, Passed:  2487, Skipped:     0, Total:  2488, Duration: 20 s - FSharp.Data.Tests.dll (net6.0)
  FSharp.Data.DesignTime.Tests -> D:\a\FSharp.Data\FSharp.Data\tests\FSharp.Data.DesignTime.Tests\bin\Release\net6.0\FSharp.Data.DesignTime.Tests.dll
Test run for D:\a\FSharp.Data\FSharp.Data\tests\FSharp.Data.DesignTime.Tests\bin\Release\net6.0\FSharp.Data.DesignTime.Tests.dll (.NETCoreApp,Version=v6.0)

@mlaily
Copy link
Contributor Author

mlaily commented Jun 10, 2022

Hello,

After investigation, the failing test is Web request's timeout is used.

The error message you highlighted about an invalid date is output by the time is omitted when zero test upon xsd schema validation failure, even when the test passes.

I'm not sure why the Web request's timeout is used test failed. We probably should rely on external services as little as possible to avoid flaky results.

 Failed Web request's timeout is used [37 ms]
  Error Message:
   System.NullReferenceException : Object reference not set to an instance of an object.
  Stack Trace:
     at FSharp.Data.Tests.Http.Web request's timeout is used() in D:\a\FSharp.Data\FSharp.Data\tests\FSharp.Data.Tests\Http.fs:line 134

image

Do you think it would be possible to simply run the build again?

@cartermp
Copy link
Collaborator

Yeah, the tests aren't ideal. Ah well! Thanks a bunch, it looks good.

@cartermp cartermp merged commit 310d1d7 into fsprojects:main Jun 11, 2022
@mlaily
Copy link
Contributor Author

mlaily commented Jun 11, 2022

Thanks a lot!

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.

Support JSON comments
2 participants