Fix CoreFn FromJSON version parsing and add test #3877
Conversation
| @@ -47,6 +47,15 @@ spec = context "CoreFnFromJsonTest" $ do | |||
| ss = SourceSpan mp (SourcePos 0 0) (SourcePos 0 0) | |||
| ann = ssAnn ss | |||
|
|
|||
| specify "should parse version" $ do | |||
| let v = Version [0, 13, 6] [] | |||
There was a problem hiding this comment.
With the old implementation, using let v = Version [0] [] here still allowed this test to pass.
paulyoung
changed the title
|
|
||
| import Language.PureScript.AST.SourcePos (SourceSpan(SourceSpan)) | ||
| import Language.PureScript.AST.Literals | ||
| import Language.PureScript.CoreFn.Ann | ||
| import Language.PureScript.CoreFn | ||
| import Language.PureScript.Docs.Types (parseVersion') |
There was a problem hiding this comment.
That dependency seems the wrong way around. Could we move that parseVersion' somewhere both the Docs and CoreFn can import from?
There was a problem hiding this comment.
No problem. Do you have any suggestions on where to put it?
There was a problem hiding this comment.
I've looked around a bit and I can't come up with anything better than just putting it into CoreFn.FromJSON. @hdgarrood any preferences?
There was a problem hiding this comment.
I feel like we should just dump this sort of stuff - stuff we kind of wished was provided by upstream libraries - into a Language.PureScript.Utils module. There are some modules in the compiler already for this sort of stuff which don’t use the Language.PureScript namespace but they really ought to, imo. But that’s probably separate... for now I don’t mind having one of these import the other. I don’t have a preference for which way around they are.
There was a problem hiding this comment.
Allright, in that case I'd prefer the function lived here. I see ide/docs/publish as downstream from the compilers PoV.
|
Thanks! |
* Fix CoreFn version parsing and add test * Move parseVersion' to CoreFn.FromJSON module
I noticed that a value of
0.13.6incorefn.jsonwas being parsed as0. This fixes that, and adds a test.This bug appears to have been around since #3049, so about 3 years 😎