purescript / registry-dev

Development work related to the PureScript Registry
https://github.com/purescript/registry
97 stars 80 forks source link

Render solver errors with backticks to prevent accidental quoting #600

Closed thomashoneyman closed 1 year ago

thomashoneyman commented 1 year ago

Wraps the solver error output in backticks to prevent accidental quoting when showing an error range that begins with >. See, for example:

https://github.com/purescript/registry/issues/249#issuecomment-1513535532

@f-f As a brief aside, this is something to keep in mind when recording logs. When notifying via GitHub we want markdown formatting; when storing a log for terminal output we don't.

f-f commented 1 year ago

Good point on the markdown formatting - then maybe we should not be using strings, but something more structured? This would be akin to what Dodo does, allowing you to use different formatters.

thomashoneyman commented 1 year ago

Incidentally, the Log and Notify effects do use Dodo and can accept a Doc directly.

f-f commented 1 year ago

Excellent - I now wonder if we can express this Markdown formatting change with a custom formatter

thomashoneyman commented 1 year ago

I'm open to exploring that. For the time being, though, I'd like to get this in — it's not a new behavior, we have backticks written into our string logs all over the place — since it's making the solver errors hard to understand right now (they appear to be saying =1.7.0 for example when it's really >=1.7.0). It's all a holdover from when everything was literally strings being thrown as Aff exceptions.

Perhaps we could make an issue to come back through the log messages to format them more reliably?

f-f commented 1 year ago

I approved, but also just realised there's a nicer way to format this: instead of backticks we could use Dodo.indent to indent this block?

In this way:

thomashoneyman commented 1 year ago

That is true, though you would need to turn the exception type from String to Doc and it may become a more pervasive change than you're thinking (hence why I've suggested a separate PR).