search

briansmith / webpki

WebPKI X.509 Certificate Validation in Rust
https://briansmith.org/rustdoc/webpki/
Other
464 stars 166 forks source link

Rename some items to conform to Rust naming conventions #210

Closed briansmith closed 3 years ago

briansmith commented 3 years ago

See the individual commit messages for details.

briansmith commented 3 years ago

@djc PTAL at this and let me know if there are any other renamings I should do.

codecov[bot] commented 3 years ago

Codecov Report

Merging #210 (b5fe1a8) into main (5fd730e) will decrease coverage by 20.96%. The diff coverage is 61.90%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #210       +/-   ##
===========================================
- Coverage   71.48%   50.52%   -20.97%     
===========================================
  Files          17       17               
  Lines        1403     1987      +584     
===========================================
+ Hits         1003     1004        +1     
- Misses        400      983      +583
Impacted Files Coverage Δ
src/calendar.rs 89.01% <0.00%> (ø)
src/error.rs 21.66% <ø> (-50.56%) :arrow_down:
src/lib.rs 1.49% <0.00%> (-13.60%) :arrow_down:
src/name/ip_address.rs 0.00% <0.00%> (ø)
src/name/verify.rs 4.06% <0.00%> (-0.05%) :arrow_down:
src/end_entity.rs 31.08% <66.66%> (-68.92%) :arrow_down:
src/der.rs 90.17% <68.75%> (ø)
src/name/dns_name.rs 61.39% <71.42%> (ø)
src/verify_cert.rs 89.17% <76.19%> (ø)
src/cert.rs 95.79% <87.50%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5fd730e...b5fe1a8. Read the comment docs.

djc commented 3 years ago

I looked over the rustdoc from 0.22.0. Here are things that stood out:

None of these are especially aggravating, though, so this is a nice improvement.

briansmith commented 3 years ago
  • Weird argument name TlsServerTrustAnchors in EndEntityCert::verify_is_valid_tls_*_cert(), though method argument names don't actually get written down in code.

If I correctly understand what you're referencing, that isn't the argument name; it's a pattern: &TlsClientTrustAnchors(trust_anchors): &TlsClientTrustAnchors.

  • The Names name for the generic argument in EndEntityCert::verify_is_valid_for_at_least_one_dns_name() also had me confused for a moment since it doesn't use the usual convention of using one letter names. In this case, maybe just write the argument type as an impl Iterator directly?

Sure, I'll switch it to impl Iterator. FWIW, I don't believe the one-letter names are a good convention, which is why I often use longer names (for lifetimes as well).

  • The spki field name in TrustAnchor stands out a little because the other fields are fully written out. I think it would be more idiomatic here to write out subject_public_key_info even though it's a mouthful.

I'll consider this when/if we have some other reason to change it. SPKI is a well-known acronym in the X.509 space, although it's fair to note that many users will have no familiarity with X.509.

  • In Error::PathLenConstraintViolated, I'd probably write out Length instead of Len.

pathLenConstraint, spelled exactly like that, is a term from the X.509 spec.

  • In Error::RequiredEkuNotFound, Eku stands out as an uncommon acronym. I might write it out as RequiredKeyUsageNotFound.

keyUsage is a different field of the certificate that has a different meaning. EKU is a very common abbreviation for people dealing with certificates.

None of these are especially aggravating, though, so this is a nice improvement.

Thanks for looking through this. I was focused on the names that affect the API but your feedback on the other names has been useful too.