Add valid location list to orphan instance error#3106
Add valid location list to orphan instance error#3106paf31 merged 4 commits intopurescript:masterfrom i-am-tom:master
Conversation
Previously, the orphan instance simply said to consider a newtype. Now, it lists all the places in the project that would be considered valid locations for the instance as well. This hopefully closes #3063.
|
What does the error message look like for: instance what :: Fail "what" |
|
Not that anyone should be writing that, but my intent is to suppose there is a prim type class for which it might make sense to make instances of. |
|
@LiamGoodacre Ha! So, originally, it would have listed |
Firstly, there's a little change to the error output in the case of there being no place for the instance except Prim. Previously, it just printed an empty list, which didn't make much sense, gramatically. Now, however, a different error is displayed to point out that there's no reasonable home for the instance. As well as this, the `typeModule` function in `TypeChecker.hs` can now deal with proxy types. Previously, it couldn't, and the tests were passing really due to an accident of laziness. Now, the tests pass, and all is well in the world.
Updates from reviewFirstly, there's a little change to the error output in the case of As well as this, the |
Thinking about it, you'll only ever see, at most, two things in the list of possible locations. So, we can intercalate the list with `or` and get a much friendlier-looking error.
src/Language/PureScript/Errors.hs
Outdated
| ] | ||
| _ -> [ Box.text $ "This instance must be declared in " | ||
| <> T.unpack formattedModules | ||
| <> ", or be defined for a newtype wrapper." |
There was a problem hiding this comment.
I'm wondering about the phrasing here; the use of 'must' makes it sound like these are strictly the only two possible solutions. However, there are potentially many other contextually dependent solutions.
There was a problem hiding this comment.
c724145#diff-2a67dc7a3cb0d6822d66486323cc9420R777 - what do you think?
| ] | ||
| , Box.vcat Box.left $ case modulesToList of | ||
| [] -> [ line "There is nowhere this instance can be placed without being an orphan." | ||
| , line "A newtype wrapper can be used to avoid this problem." |
There was a problem hiding this comment.
Seems like we'd also probably need a link to what the hell a newtype wrapper is and how to define it?
There was a problem hiding this comment.
Spoke to @paf31, suggested that this level of detail would be better suited to the wiki. We're going to talk a bit more about this later, and I'll probably make another docs PR :)
Previously, the error mentioned that the instance "must" be declared in one of a set of places, which @goodacre.liam (rightly) noted would imply that there aren't other, equally valid, solutions. The wording has been altered to address this.
|
🎉 |
|
Thanks! |
* Add valid location list to orphan instance error Previously, the orphan instance simply said to consider a newtype. Now, it lists all the places in the project that would be considered valid locations for the instance as well. This hopefully closes #3063. * Updates from review Firstly, there's a little change to the error output in the case of there being no place for the instance except Prim. Previously, it just printed an empty list, which didn't make much sense, gramatically. Now, however, a different error is displayed to point out that there's no reasonable home for the instance. As well as this, the `typeModule` function in `TypeChecker.hs` can now deal with proxy types. Previously, it couldn't, and the tests were passing really due to an accident of laziness. Now, the tests pass, and all is well in the world. * Tidy the orphan instance error further. Thinking about it, you'll only ever see, at most, two things in the list of possible locations. So, we can intercalate the list with `or` and get a much friendlier-looking error. * Adjust wording on error feedback Previously, the error mentioned that the instance "must" be declared in one of a set of places, which @goodacre.liam (rightly) noted would imply that there aren't other, equally valid, solutions. The wording has been altered to address this.
* Add valid location list to orphan instance error Previously, the orphan instance simply said to consider a newtype. Now, it lists all the places in the project that would be considered valid locations for the instance as well. This hopefully closes purescript#3063. * Updates from review Firstly, there's a little change to the error output in the case of there being no place for the instance except Prim. Previously, it just printed an empty list, which didn't make much sense, gramatically. Now, however, a different error is displayed to point out that there's no reasonable home for the instance. As well as this, the `typeModule` function in `TypeChecker.hs` can now deal with proxy types. Previously, it couldn't, and the tests were passing really due to an accident of laziness. Now, the tests pass, and all is well in the world. * Tidy the orphan instance error further. Thinking about it, you'll only ever see, at most, two things in the list of possible locations. So, we can intercalate the list with `or` and get a much friendlier-looking error. * Adjust wording on error feedback Previously, the error mentioned that the instance "must" be declared in one of a set of places, which @goodacre.liam (rightly) noted would imply that there aren't other, equally valid, solutions. The wording has been altered to address this.
Previously, the orphan instance simply said to consider a newtype. Now, it lists all the places in the project that would be considered valid locations for the instance as well. This hopefully closes #3063, and looks a little something like: