HighlightingAssets: Make .syntaxes() and .syntax_for_file_name() failable #1755
Conversation
src/assets.rs
Outdated
| .and_then(|l| self.get_syntax_set().find_syntax_by_first_line(&l)) | ||
| .and_then(|l| { | ||
| self.get_syntax_set() | ||
| .ok() |
There was a problem hiding this comment.
This will conflate a missing syntax_set with the non-availability of a first line syntax. Maybe it's better to use an explicit unwrap here?
There was a problem hiding this comment.
Similar for the other .ok() calls.
There was a problem hiding this comment.
Hmm, wouldn't it be better to change to Result<Option> instead? So that we robustly handle all three cases:
- Failure to look up
- Nothing was found
- Something was found
There was a problem hiding this comment.
It depends whether or not the get_syntax_set method can fail at this particular place. Based on the .unwrap() calls in similar methods below, I thought it could not fail here. In that case I would have preferred .unwrap() or .expect(). If it can indeed fail, Result<Option<_>> is definitely the way to go. You will probably need one of my favorite helper functions (sequence in Haskell)
There was a problem hiding this comment.
Aha, you probably missed that my .unwrap()s are in mod tests { ...? I think it is (more) OK to take shortcuts in tests. (I also do .unwrap() in PrettyPrinter, but there is can never fail, as motivated in the added comment.)
I decided to go full robustness, and that it was not enough to handle rm ~/.cache/bat/syntaxes.bin. I also want to robustly (meaning: giving helpful message to the user, and not panicing) handle echo garbage >! ~/.cache/bat/syntaxes.bin. So my proposed quickfix in the lazy-load PR was not enough.
(I'm not going to bother protecting against flipped bits in the bat binary itself though...)
Anyway, I'll go through which code paths with Option that can never fail (because of them never being the first call to get_syntax_set()), and adjust accordingly. Thanks for the code review!
There was a problem hiding this comment.
I've cascaded the return of Result now to all necessary places, and all regression tests pass, so would be great if you could take a second look.
Edit: I've also verified now that basing #1747 on this change gets rid of the panics for the edge-cases 🎉
There was a problem hiding this comment.
Aha, you probably missed that my
.unwrap()s are inmod tests { ...? I think it is (more) OK to take shortcuts in tests. (I also do .unwrap() inPrettyPrinter, but there is can never fail, as motivated in the added comment.)
I saw the unwrap in PrettyPrinter and thought that it might apply to these other situations. Thanks for clarifying.
Or rather, introduce new versions of these methods and deprecate the old ones. This is preparation to enable robust and user-friendly support for lazy-loading. With lazy-loading, we don't know if the SyntaxSet is valid until after we try to use it, so wherever we try to use it, we need to return a Result. See discussion about panics in sharkdp#1747.
Enselic
force-pushed
the
introduce-failable-version-of-syntaxes
branch
from
d8cc199
to
b6cecb5
Compare
Enselic
changed the title
Or rather, introduce new versions of these methods and deprecate the old ones.
This is preparation to enable robust and user-friendly support for lazy-loading.
With lazy-loading, we don't know if the SyntaxSet is valid until after we try to
use it, so wherever we try to use it, we need to return a Result. See discussion
about panics in #1747.