Error message shown for ProcfileParsingError is not accurate
#74
Assignees
Comments
edmorley
changed the title
ProcfileBuildpackError::ProcfileParsingError isn't really accurateProcfileParsingError is not accurate
edmorley
added a commit
that referenced
this issue
Jun 20, 2022
There is currently only one error variant for `ProcfileParsingError`, an error that only occurs if it was not possible to compile the regex used to parse a `Procfile`. This regex is buildpack-supplied, so if such an error occurs, it's always an internal buildpack error. However previously a user-facing error message was shown, that told the user to check their `Procfile` was valid, which has no bearing on whether the regex will compile. I believe part of the reason for this confusion over error messages is that the error message handling was split across multiple files due to the use of `thiserror` meaning that the display representation could be leant upon, without actually looking closely at what was being displayed. As such, I've removed `thiserror`, since I think the error handling is easier to reason about without it. Fixes #74. GUS-W-11318868.
edmorley
added a commit
that referenced
this issue
Jun 20, 2022
There is currently only one error variant for `ProcfileParsingError`, an error that only occurs if it was not possible to compile the regex used to parse a `Procfile`. This regex is buildpack-supplied, so if such an error occurs, it's always an internal buildpack error. However previously a user-facing error message was shown, that told the user to check their `Procfile` was valid, which has no bearing on whether the regex will compile. Rather than just correct the error message, the error has been removed and instead `.expect()` used, since such regex errors will not happen in practice, given they will be caught by both the `invalid_regex` Clippy lint and the buildpack's tests. I believe part of the reason for this confusion over error messages is that the error message handling was split across multiple files due to the use of `thiserror` meaning that the display representation could be leant upon, without actually looking closely at what was being displayed. As such, I've removed `thiserror`, since I think the error handling is easier to reason about without it. Fixes #74. GUS-W-11318868.
edmorley
added a commit
that referenced
this issue
Jun 20, 2022
There is currently only one error variant for `ProcfileParsingError`, an error that only occurs if it was not possible to compile the regex used to parse a `Procfile`. This regex is buildpack-supplied, so if such an error occurs, it's always an internal buildpack error. However previously a user-facing error message was shown, that told the user to check their `Procfile` was valid, which has no bearing on whether the regex will compile. Rather than just correct the error message, the error has been removed and instead `.expect()` used, since such regex errors will not happen in practice, given they will be caught by both the `invalid_regex` Clippy lint and the buildpack's tests. I believe part of the reason for this confusion over error messages is that the error message handling was split across multiple files due to the use of `thiserror` meaning that the display representation could be leant upon, without actually looking closely at what was being displayed. As such, I've removed `thiserror`, since I think the error handling is easier to reason about without it. Fixes #74. GUS-W-11318868.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently for
ProcfileBuildpackError::ProcfileParsingError, the following error message is shown:https://github.com/heroku/procfile-cnb/blob/683de9b9877957ca0a5e6ba6733059cb55b5c4d9/src/error.rs#L32-L40
That error message suggests the error must be due to a problem in the provided
Procfile(ie a user error).However, at the moment the only way parsing a
Procfilecan error is due to a regex compilation error, which can only be due to a buildpack bug instead:https://github.com/heroku/procfile-cnb/blob/683de9b9877957ca0a5e6ba6733059cb55b5c4d9/src/procfile.rs#L66-L69
https://github.com/heroku/procfile-cnb/blob/683de9b9877957ca0a5e6ba6733059cb55b5c4d9/src/procfile.rs#L36-L43
Now this may change in the future if #73 is implemented (at which point there will be more
ProcfileParsingErrorvariants) - however even then, we would still likely want to differentiate between "buildpack internal error" and "user error" - so it seems either way that the error message needs adjusting.cc @schneems @Malax
GUS-W-11318868.
The text was updated successfully, but these errors were encountered: