Make FromJSON instance for Qualified backwards compatible#4403
Conversation
pete-murphy
force-pushed
the
pm/qualified-compat-fromjson-instance
branch
from
06dc567 to
36ae105
Compare
| instance FromJSON a => FromJSON (Qualified a) where | ||
| parseJSON v = byModule <|> bySourcePos | ||
| parseJSON v = byModule <|> bySourcePos <|> byMaybeModuleName' | ||
| where | ||
| byModule = do | ||
| (mn, a) <- parseJSON2 v | ||
| pure $ Qualified (ByModuleName mn) a | ||
| bySourcePos = do | ||
| (ss, a) <- parseJSON2 v | ||
| pure $ Qualified (BySourcePos ss) a | ||
| byMaybeModuleName' = do | ||
| (mn, a) <- parseJSON2 v | ||
| pure $ Qualified (byMaybeModuleName mn) a |
There was a problem hiding this comment.
Maybe this
instance FromJSON a => FromJSON (Qualified a) where
parseJSON v = byModule <|> bySourcePos <|> byNullSourcePos
where
-- ...
byNullSourcePos = do
(Nothing :: Maybe SourcePos, a) <- parseJSON2 v
pure $ Qualified ByNullSourcePos awould be preferable? The byMaybeModuleName is redundant I think, should only reach here if the array has null in first position.
There was a problem hiding this comment.
Also wondering if the ToJSON instance should map ByNullSourcePos to null (currently maps to [0,0]), so that decoding then encoding is a round trip.
There was a problem hiding this comment.
I think just updating the SourcePos instances seems simpler 4d70482 Oops, it seemed more elegant, but now failing some tests Nevermind nevermind, I think my local branch was just in a bad state.
pete-murphy
force-pushed
the
pm/qualified-compat-fromjson-instance
branch
from
36ae105 to
1ae1490
Compare
pete-murphy
changed the title
pete-murphy
force-pushed
the
pm/qualified-compat-fromjson-instance
branch
from
4d70482 to
1ae1490
Compare
| toJSON SourcePos{..} = | ||
| A.toJSON [sourcePosLine, sourcePosColumn] | ||
| toJSON = \case | ||
| NullSourcePos -> A.Null |
There was a problem hiding this comment.
This is a breaking change in that anything consuming this JSON now needs to handle the null case where it wouldn't have had to previously. If this data type occurs in CoreFn, that will be a problem. I think this is worse than losing the roundtrip property. We should have the roundtrip property for JSON produced by the current version, but losing it for JSON that can only be produced by previous versions is fine, I think.
There was a problem hiding this comment.
That makes sense, I've reverted to previous commit.
pete-murphy
force-pushed
the
pm/qualified-compat-fromjson-instance
branch
from
4d70482 to
1ae1490
Compare
3 participants
Make
FromJSONinstance forQualifiedbackwards compatiblePrior to #4293,
Qualifiedwas encoded to JSON such thatThe type of
Qualifiedhas changed so thatnullno longer appears in JSON output, but for sake of backwards-compatibility with JSON that was produced prior to those changes (pre-v0.15.2), we need to acceptnull, which will be interpreted asQualified ByNullSourcePos.Relates to: purescript/pursuit#470 (comment)
Checklist: