Add context in name validation errors

[djc]

This pretty much achieves my goals and is probably workable but not quite pretty:

• No Copy for Error means a semver-incompatible change
  • Semver-incompatible changes in webpki specifically are relatively manageable since most users don't depend on it directly
• A little strange to get different errors for alloc vs no alloc?

I can fix test failures etc if we think this is worth doing.

[ctz]

I think this is definitely worth doing. In the future we could also do similar treatment to:

• CertExpired
• CertNotValidYet
• CrlExpired

(with the current and cert times)

The other future improvement would be to have a more detailed fmt::Display, which could say, perhaps:

certificate is not valid for "example.com".
instead, the certificate contained "*.example.com", "foobar.org".

[codecov]

Codecov Report

Attention: Patch coverage is 97.67442% with 4 lines in your changes missing coverage. Please review.

Project coverage is 97.30%. Comparing base (5dc0926) to head (44fc4a6).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/subject_name/dns_name.rs	94.28%	2 Missing ⚠️
src/error.rs	0.00%	1 Missing ⚠️
src/subject_name/ip_address.rs	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
+ Coverage   97.23%   97.30%   +0.07%     
==========================================
  Files          20       20              
  Lines        4225     4343     +118     
==========================================
+ Hits         4108     4226     +118     
  Misses        117      117              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[djc]

I think this is definitely worth doing. In the future we could also do similar treatment to:

• CertExpired
• CertNotValidYet
• CrlExpired

(with the current and cert times)

The other future improvement would be to have a more detailed fmt::Display, which could say, perhaps:

certificate is not valid for "example.com".
instead, the certificate contained "*.example.com", "foobar.org".

Good ideas! Have not implemented them yet, figure it makes sense to get this in first as a proof point and to iron out the pattern here.

[cpu]

Here's some initial feedback. Overall I think it's 👍

[djc]

Fixed the test failures and tacked on some refactoring.

[cpu]

The error context changes LGTM.

I'm a little bit nervous about the NameIterator simplification at the end w.r.t the directory_name bool change. I think it's probably OK but want to review that part of the diff with fresh eyes before I +1 the whole PR. I find the pre-existing code hard to reason about when I haven't been working in this repo recently.

If you want to pull refactoring out into a second PR to land the error improvement sooner that's OK with me too.

[cpu]

To see the specific tasks where the Asana app for GitHub is being used, see below:
https://app.asana.com/0/0/1209099649554591
https://app.asana.com/0/0/1209099649554598

Is this something you're using to track your F/OSS work? FWIW the links are 404s for me as an unauthed user so I'm not sure it's useful for Asana to be putting them in the PR desc.

[djc]

Is this something you're using to track your F/OSS work? FWIW the links are 404s for me as an unauthed user so I'm not sure it's useful for Asana to be putting them in the PR desc.

Yeah, trying out Asana to track my personal tasks and its GitHub integration turned out to be more bidirectional than I thought. Will remove it.

[djc]

If you want to pull refactoring out into a second PR to land the error improvement sooner that's OK with me too.

That sounds like a good idea!

[djc]

I'm a little bit nervous about the NameIterator simplification at the end w.r.t the directory_name bool change.

It's gone for now, will bring it back in a separate PR after this is merged.

[cpu]

Nice :-)

[djc]

Will wait to see if @ctz has any more detailed feedback.

[ctz]

(beach review: lgtm!)