Skip to content

Conversation

@npmccallum
Copy link
Contributor

@npmccallum npmccallum commented Feb 11, 2022

This PR depends on:

Yes, there's a lot of commits here. However, this PR actually represents a significant reduction in code in the x509 crate without loss of functionality. All types are derived and there are no custom implementations.

@npmccallum npmccallum force-pushed the cleanup branch 4 times, most recently from c7478fb to 408abf9 Compare February 11, 2022 06:59
der/Cargo.toml Outdated
rust-version = "1.57"

[dependencies]
flagset = { version = "^0.4.3", optional = true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this PR incorporates #412 (and I'm guessing some of the others as well)?

BTW the ^ is redundant as Cargo defaults to that type of range comparison:

Suggested change
flagset = { version = "^0.4.3", optional = true }
flagset = { version = "0.4.3", optional = true }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tarcieri Yes. The description field at the top of the PR has check boxes for each of the dependency PRs.

@npmccallum
Copy link
Contributor Author

I have moved the early part of this PR into #422 for independent review.

@npmccallum npmccallum force-pushed the cleanup branch 3 times, most recently from 484475e to 46d4a10 Compare February 11, 2022 21:21
@npmccallum
Copy link
Contributor Author

@tarcieri @carl-wallace All the dependencies for this PR have now been merged. What remains in this PR is just moving types around to create a better hierarchical module structure and improving documentation. I think the best way to review this PR is to check it out and run cargo doc to inspect the layout.

@npmccallum
Copy link
Contributor Author

@tarcieri Any progress on this? I have more types to add, but I want to add them to the post-reorg structure.

@npmccallum npmccallum force-pushed the cleanup branch 3 times, most recently from 9a210fb to a50dc37 Compare February 16, 2022 13:06
Signed-off-by: Nathaniel McCallum <[email protected]>
Signed-off-by: Nathaniel McCallum <[email protected]>
@@ -1,4 +1,4 @@
#![no_std]
#![cfg_attr(not(test), no_std)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a specific reason for this? I couldn't see why it was needed.

The main problem with doing this is potential build failures for std-dependent (or std prelude-dependent) features that you only see when doing cargo build that are masked when doing cargo test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it because tests didn't run without it. But I forget why. I can investigate this afternoon.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will go ahead and approve this. Can take a look at the tests separately.

@tarcieri tarcieri merged commit 0018228 into RustCrypto:master Feb 16, 2022
@npmccallum npmccallum deleted the cleanup branch March 10, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants