Add review for yap v0.7.2
This commit is contained in:
parent
4d81f12f80
commit
ff2cb2fd4e
|
@ -23,3 +23,69 @@ comment: |-
|
||||||
mJagAUfgDd8cusPe6ha009XxnQ2fCLzHl17T3gLpebo1RgIWmW6b41oezwkxbQeRh2i4Py7m1olDvu5fUSPsBg
|
mJagAUfgDd8cusPe6ha009XxnQ2fCLzHl17T3gLpebo1RgIWmW6b41oezwkxbQeRh2i4Py7m1olDvu5fUSPsBg
|
||||||
----- END CREV PROOF -----
|
----- END CREV PROOF -----
|
||||||
|
|
||||||
|
----- BEGIN CREV PROOF -----
|
||||||
|
kind: package review
|
||||||
|
version: -1
|
||||||
|
date: "2022-06-27T22:44:05.352091075+02:00"
|
||||||
|
from:
|
||||||
|
id-type: crev
|
||||||
|
id: YdnEoYtqvbBGv0hhENLDUYmc3tNfm5V5NIG5hCovHyM
|
||||||
|
url: "https://git.jcg.re/jcgruenhage/crev-proofs.git"
|
||||||
|
package:
|
||||||
|
source: "https://crates.io"
|
||||||
|
name: yap
|
||||||
|
version: 0.7.2
|
||||||
|
revision: 6ee4dfc176a5690063e391603b2b6ad2bd5b2514
|
||||||
|
digest: K5Fu1zb_agTCEM2nYx2SyUDzBPRJ4EAFi8aK1eubGMM
|
||||||
|
review:
|
||||||
|
thoroughness: high
|
||||||
|
understanding: high
|
||||||
|
rating: positive
|
||||||
|
alternatives:
|
||||||
|
- source: "https://crates.io"
|
||||||
|
name: nom
|
||||||
|
comment: |-
|
||||||
|
Simple enough parser combinator library that comes with most of the bells and
|
||||||
|
whistles I was looking for. I've noticed a few minor things that could use
|
||||||
|
improving, but nothing critical.
|
||||||
|
|
||||||
|
First of all, why did I review this? I implemented parsing a specific HTTP
|
||||||
|
auth header credential, which is non-trivial to do with just string splitting.
|
||||||
|
I had gathered some experience with `yap` during last years Advent of Code,
|
||||||
|
and I'm relatively happy with the implementation now. The only thing I was
|
||||||
|
missing is something like `Tokens::optional` that was based on `Result`
|
||||||
|
instead of `Option`. I'll make sure to contribute that later though.
|
||||||
|
|
||||||
|
As for problems I've found while reviewing this crate:
|
||||||
|
|
||||||
|
- There's one sound use of unsafe, that could probably be avoided, just for
|
||||||
|
good measure. It's about going from a `str` to the next `char`. It
|
||||||
|
increases a counter by one, and then by another one until it's at a char
|
||||||
|
boundary. This is explicitly checked before it then does
|
||||||
|
`str::get_unchecked`, which is the unsafe call. It seems sound to me, but
|
||||||
|
without tests this isn't a great look.
|
||||||
|
|
||||||
|
- The formatting is a bit off in some places. A few extra spaces, nothing
|
||||||
|
big, just made reviewing a bit more of a chore.
|
||||||
|
|
||||||
|
- The `logos` dev-dependency doesn't seem to be used anywhere. As `logos` is
|
||||||
|
quite wide-spread, it being included in yap is not much of a supply chain
|
||||||
|
safety loss, but when it's not used it should probably be dropped anyway.
|
||||||
|
|
||||||
|
Things I specifically liked:
|
||||||
|
|
||||||
|
- The documentation is amazing. Every single thing one could hope for has a
|
||||||
|
doc comment, and the documentation really goes above and beyond to
|
||||||
|
demonstrate how yap can be used.
|
||||||
|
|
||||||
|
- Leveraging iterators was a very good choice. Using `yap` feels super
|
||||||
|
natural, and even without any prior parser-combinator experience, I was
|
||||||
|
able to quickly get started. Not using regular expressions and string
|
||||||
|
splitting feels really good.
|
||||||
|
|
||||||
|
In general, I'm extremely happy with yap, and even happier now that I've done
|
||||||
|
my due diligence with regards to supply chain security. Thanks to the authors.
|
||||||
|
----- SIGN CREV PROOF -----
|
||||||
|
wWTVLbr4D3hgMVVz8g7Q5GQdiSXO-_FlbVdQ0rWJ7jMNValXnZGLnDNPiM5gZFx9-JCw95pmZOpRaHMjBlrnDw
|
||||||
|
----- END CREV PROOF -----
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue