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

JsonProvider.Load(value: JsonValue) #1424

Merged
merged 2 commits into from
Jan 12, 2022

Conversation

gbtb
Copy link
Contributor

@gbtb gbtb commented Jan 8, 2022

Added overload of Load method on JsonProvider to support combining JsonProviders without need for additional conversion to and from string. This should fix #1309

Copy link
Collaborator

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Looks good overall, curious about the test change

Comment on lines +32 to +36
#if DEBUG
"Debug"
#else
"Release"
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

@gbtb gbtb Jan 12, 2022

Choose a reason for hiding this comment

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

Maybe it's just my workflow, but I rarely build projects in a Release configuration while developing. So I've ran tests in debug mode and was a bit puzzled when all of them failed at once, because of missing Release-build dll . So I've thought synchronizing configurations of test assembly and required assembly would be convenient

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool!. I think this is fine. I had to make a change when building/testing for .NET 5 to run tests in release, since at the time the compiler could stack overflow. That may no longer be the case (there have been a years' worth of compiler improvements since then). This is probably a better way to do it.

@cartermp cartermp merged commit d37a75c into fsprojects:main Jan 12, 2022
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 loading data from JsonValue for JsonProvider.
2 participants