Skip to content

feat: introduce ID crate #330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 7, 2025
Merged

feat: introduce ID crate #330

merged 2 commits into from
Mar 7, 2025

Conversation

daniel-noland
Copy link
Collaborator

See docs in crate for details

I hope to make this useful in other crates shortly.

@daniel-noland daniel-noland added the enhancement New feature or request label Mar 4, 2025
@daniel-noland daniel-noland self-assigned this Mar 4, 2025
@daniel-noland daniel-noland force-pushed the pr/daniel-noland/id branch 3 times, most recently from f7b298c to 2e31c2d Compare March 4, 2025 20:39
@daniel-noland daniel-noland marked this pull request as ready for review March 4, 2025 20:57
@daniel-noland daniel-noland requested a review from a team as a code owner March 4, 2025 20:57
@daniel-noland daniel-noland requested review from mvachhar and removed request for a team March 4, 2025 20:57
@daniel-noland daniel-noland force-pushed the pr/daniel-noland/id branch 2 times, most recently from 99f4dfe to c3da646 Compare March 4, 2025 23:45
id/src/lib.rs Outdated
Comment on lines 111 to 113
/// if you need to use `MySpecialType` instead of [`Uuid`] for your special type of tagged type.
/// </div>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// if you need to use `MySpecialType` instead of [`Uuid`] for your special type of tagged type.
/// </div>
/// if you need to use `MySpecialType` instead of [`Uuid`] for your special type of tagged type.
/// </div>

https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html?highlight=warning#adding-a-warning-block:

Please note that if you want to put markdown in the HTML tag and for it to be interpreted as such, you need to have an empty line between the HTML tags and your markdown content.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

For unknown reasons the doctests crash on the id crate in CI.

I don't have time to debug at the moment.

I note we don't run doctests anywhere else at the moment; dataplane's pipeline module has examples, but the package is a binary and the module is not public, so its examples aren't run as part of the doctests (rust-lang/rust#50784).

The error says:

Couldn't run the test: Permission denied (os error 13) - maybe your tempdir is mounted with noexec?

So maybe we should try removing the noexec in the justfile? I think you had that in a PR recently, didn't you? Or was it a previous iteration of this one, maybe?

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

The code looks OK to me, but could you provide more details on the use case you have in mind, please?

@mvachhar
Copy link
Contributor

mvachhar commented Mar 7, 2025

The code looks OK to me, but could you provide more details on the use case you have in mind, please?

You can take a look at #320 where I will move from my internal version of this to a similar construct. But perhaps a better description in the PR is warranted.

@daniel-noland
Copy link
Collaborator Author

The code looks OK to me, but could you provide more details on the use case you have in mind, please?

Sure.

The motivation is to distinguish different types of IDs which we expect to use in configuration.

I am confident we will end up with multiple tables of configuration data, and keeping the IDs of VPCs compile time distinct from the IDs of nat rules can make things easier and less error-prone. A function which returns or accepts Id<Vpc> is clearer than one which just returns Id. Id of what?

Copy link
Contributor

@mvachhar mvachhar left a comment

Choose a reason for hiding this comment

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

Let's give a better PR message for motivation and then I am good to merge this. I'll probably add some stuff to this crate for mapping IDs to human readable strings.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

OK sounds good, thanks a lot!

@qmonnet
Copy link
Member

qmonnet commented Mar 7, 2025

We'll need to take a look into these doctests issue at some point though, or we won't be able to test examples in the docs 😢

@mvachhar
Copy link
Contributor

mvachhar commented Mar 7, 2025

The motivation is to distinguish different types of IDs which we expect to use in configuration.

I am confident we will end up with multiple tables of configuration data, and keeping the IDs of VPCs compile time distinct from the IDs of nat rules can make things easier and less error-prone. A function which returns or accepts Id<Vpc> is clearer than one which just returns Id. Id of what?

In the future, we can put the motivation in the commit log. I don't think it is worth holding up this PR just for that though. Something to keep in mind for the future.

@mvachhar mvachhar enabled auto-merge March 7, 2025 16:32
See docs in crate for details

Signed-off-by: Daniel Noland <[email protected]>
For unknown reasons the doctests crash on the id crate in CI.

I don't have time to debug at the moment.

Hopefully this change can be reverted later.

Signed-off-by: Daniel Noland <[email protected]>
@mvachhar mvachhar added this pull request to the merge queue Mar 7, 2025
Merged via the queue into main with commit 5ed9413 Mar 7, 2025
16 checks passed
@mvachhar mvachhar deleted the pr/daniel-noland/id branch March 7, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants