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 FromJSON instance for better error msgs #1882

Merged
merged 1 commit into from
Jun 1, 2024
Merged

Fix FromJSON instance for better error msgs #1882

merged 1 commit into from
Jun 1, 2024

Conversation

nitinprakash96
Copy link
Collaborator

Before:

ghci> let s = "[{ \"radius\": 2, \"density\": \"x\"}]"
ghci> eitherDecodeStrict s :: Either String GrowthSpread
Left "Error in $: parsing Growth failed, expected Object, but encountered Array

The error msg above is misleading because we weren't trying to parse Growth datatype but GrowthSpread instead.

After:

ghci> let s = "[{ \"radius\": 2, \"density\": \"x\"}]"
ghci> eitherDecodeStrict s :: Either String GrowthSpread
Left "Error in $: parsing GrowthSpread failed, expected Object, but encountered Array"

@nitinprakash96 nitinprakash96 marked this pull request as ready for review May 31, 2024 06:09
@nitinprakash96
Copy link
Collaborator Author

A couple of things that I've noticed:

  • names in withObject, withText etc do not correspond 1-1 with the datatype. This can be misleading when dealing with error msgs like in the description of the PR.
  • Manually written FromJSON instances should have manually written ToJSON instances. This enables round trip tests.
    Here is an example of not doing so:
ghci> let s = "{\"radius\": 2, \"density\": 0.3}"
ghci> eitherDecodeStrict s :: Either String GrowthSpread
Right (GrowthSpread {spreadRadius = 2, spreadDensity = 0.3})
ghci>
ghci> let spread = GrowthSpread 2 0.3
ghci> encode spread
"{\"spreadDensity\":0.3,\"spreadRadius\":2}" -- different than what the input was

Having written it manually gives us more control over the JSON in question. Is there a reason why we do not do so?

@nitinprakash96 nitinprakash96 requested a review from byorgey May 31, 2024 17:18
@byorgey
Copy link
Member

byorgey commented May 31, 2024

@nitinprakash96 , no, there's no reason to have a manually written FromJSON instance but an autogenerated ToJSON instance. If we can't round-trip the encoding then that is definitely a bug. In fact I just fixed another such bug recently, see #1870. I also added a ticket to test round-tripping of JSON instances in our CI, since currently there's nothing to catch things like this: #1869. Currently, we don't really use the ToJSON instance for anything like GrowthSpread in a scenario description (we only read them from the data files in data/scenarios). But we will eventually need them once we implement #50.

@nitinprakash96
Copy link
Collaborator Author

Makes sense. Let me see if I can get round tripping to work in the test suite.

@nitinprakash96 nitinprakash96 added the merge me Trigger the merge process of the Pull request. label Jun 1, 2024
@mergify mergify bot merged commit 4a6f413 into main Jun 1, 2024
11 checks passed
@mergify mergify bot deleted the nitin/patch branch June 1, 2024 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants