Correct the error message shown for invalid regex errors #76
Conversation
edmorley
force-pushed
the
edmorley/correct-parsing-error-message
branch
from
d70d9e4 to
a3b5bf3
Compare
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
force-pushed
the
edmorley/correct-parsing-error-message
branch
from
a3b5bf3 to
3745d65
Compare
| ProcfileParsingError(#[from] ProcfileParsingError), | ||
| #[error("Procfile conversion error: {0}")] | ||
| ProcfileConversionError(#[from] ProcfileConversionError), | ||
| Parsing(ProcfileParsingError), |
There was a problem hiding this comment.
FWIW, nit: I like having the name of the enum variant being the same name as the error it wraps if there is no additional context to be added.
There was a problem hiding this comment.
So for my Python CNB I've tried to avoid redundancy in the enum variant names where possible. The enum type itself is named ProcfileBuildpackError, so by definition we know it's Procfile related, and also they are all errors. Having shorter names then makes the nested matches slightly more readable (all of the variables starting with Procfile... were blurring into one).
However I can also see that having the names match may be appealing too.
| Regex::new("\\r\\n?").map_err(ProcfileParsingError::InvalidRegex)?; | ||
| let re_multiple_newline = | ||
| Regex::new("\\n*\\z").map_err(ProcfileParsingError::RegexError)?; | ||
| Regex::new("\\n*\\z").map_err(ProcfileParsingError::InvalidRegex)?; | ||
|
|
||
| // https://github.com/heroku/codon/blob/2613554383cb298076b4a722f4a1aa982ad757e6/lib/slug_compiler/slug.rb#L538-L545 | ||
| let re_procfile_entry = Regex::new("^[[:space:]]*([a-zA-Z0-9_-]+):?\\s+(.*)[[:space:]]*") | ||
| .map_err(ProcfileParsingError::RegexError)?; | ||
| .map_err(ProcfileParsingError::InvalidRegex)?; |
There was a problem hiding this comment.
I would even go so far and say these arent ProcfileParsingErrors. When they occur, it's clearly a programmer error on procfile-cnbs part. Those don't have to be errors at runtime IMO. This is one of the rare occurrences where I think panicing is fine.
I suggest we remove the InvalidRegex variant and call expect instead of mapping. Clippy will actually verify that the regex in our source is valid/compilable - so actual panics should never happen if our CI setup is correct. https://rust-lang.github.io/rust-clippy/master/#invalid_regex
This would make things much easier and would've prevented the incorrect error message in the first place.
There was a problem hiding this comment.
I also found https://github.com/Canop/lazy-regex which does compile-time regex validation - however I'm slightly reluctant to switch to a less popular crate that adds another layer of abstraction around regex.
Agree about Clippy validation, and I'd be fine with .expect() with a comment too.
There was a problem hiding this comment.
I'll mark my review as "changes required" then :)
There was a problem hiding this comment.
So once those errors are removed, impl FromStr for Procfile never returns an error.
As such, I was thinking I could just use type Err = Infallible (std::convert::Infallible).
However it seems conversion from infallible (without using eg unwrap()) is still pretty rough around the edges, eg this unstabilised feature:
rust-lang/rust#61695
One option would be to drop the FromStr and just use the infallible impl From<&str> for Procfile instead? The API would at least more closely match reality then. However if we ever make things more strict #73 then that would change again (though given private APIs doesn't really matter I suppose).
Thoughts?
There was a problem hiding this comment.
From<&str> sounds good to me - if it's really infallible there is no need to encode errors.
There was a problem hiding this comment.
So thinking about this some more, rather than switch from FromStr to impl From<&str> for Procfile now, only to then have to switch to either TryFrom or back to FromStr in the future (when we fix #73), I'm leaning towards using an empty error variant (pub enum ProcfileParsingError {}) instead (into which we can then add the future errors).
There was a problem hiding this comment.
From an API standpoint I think that models the situation reasonably well: "a function that should be treated as fallible, but right now there are zero implemented modes of failure".
There was a problem hiding this comment.
Works for me too. Whatever models reality the best - if we expect there to be errors soon we can keep TryFrom. :)
|
Superseded by #77. |
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 aProcfile.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
Procfilewas 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
thiserrormeaning 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.