Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ProcfileParsingError::RegexError as a user-facing error #77

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

edmorley
Copy link
Member

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. See the discussion in:
#76 (comment)

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.

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 edmorley self-assigned this Jun 20, 2022
@edmorley edmorley marked this pull request as ready for review June 20, 2022 15:24
@edmorley edmorley requested a review from a team as a code owner June 20, 2022 15:24
Copy link
Member

@Malax Malax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@edmorley edmorley merged commit 33234be into main Jun 20, 2022
@edmorley edmorley deleted the edmorley/rm-parsing-regex-error branch June 20, 2022 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message shown for ProcfileParsingError is not accurate
2 participants