Refactor [] to NonEmpty in Quasi module#1193
Conversation
danbroooks
changed the title
|
I've fixed CI, mind rebasing and trying again? |
| parse ps = maybe [] (parseLines ps) . preparse | ||
|
|
||
| preparse :: Text -> [Line] | ||
| preparse :: Text -> Maybe (NonEmpty Line) |
There was a problem hiding this comment.
Maybe (NonEmpty a) is equivalent to [a]. What's the purpose of this change?
There was a problem hiding this comment.
Well its just so that parseLines can take a NonEmpty value, but it might make sense to make that call to nonEmpty within parse actually and then leave preparse alone.
I guess alternatively though you could say preparse can potentially fail to return anything of interest, and thus that is represented as a Nothing value here. You can say the same thing about parse as well, though obviously that needs to remain as [].
I'm happy to move nonEmpty directly into parse though if that makes more sense 👍
There was a problem hiding this comment.
I think I'm just not sure about the benefit of this particular [] -> NonEmpty change. We don't appear to have any behavior that assumes or depends on [] being non-empty somewhere with this set of code.
But, it's also not a breaking change, and it seems like it makes sense semantically in the parsing section here.
| parse ps = maybe [] (parseLines ps) . preparse | ||
|
|
||
| preparse :: Text -> [Line] | ||
| preparse :: Text -> Maybe (NonEmpty Line) |
There was a problem hiding this comment.
I think I'm just not sure about the benefit of this particular [] -> NonEmpty change. We don't appear to have any behavior that assumes or depends on [] being non-empty somewhere with this set of code.
But, it's also not a breaking change, and it seems like it makes sense semantically in the parsing section here.
|
Since this is a refactoring-only change, isn't exposed to the public interface, and is covered entirely by the tests, I'm going to merge without requiring a changelong entry. If you want to put a changelog entry in for the sweet sweet credit, feel free to push it up. I'll merge this sometime in the next few days. Thanks! |
2 participants
A refactoring contribution towards #1061