Skip to content
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

Provide uuid macro without dependencies #566

Merged
merged 2 commits into from Nov 16, 2021

Conversation

Nugine
Copy link
Contributor

@Nugine Nugine commented Nov 16, 2021

I'm submitting a feature

Description

#556 (comment)

Related Issue(s)

resolves #556

@KodrAus
Copy link
Member

KodrAus commented Nov 16, 2021

cc @QnnOkabayashi

src/macros.rs Show resolved Hide resolved
Copy link
Contributor

@QnnOkabayashi QnnOkabayashi left a comment

Choose a reason for hiding this comment

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

At this point, we should change the feature gate to something like "macro-diagnostics". I also personally think that it should be on by default and require opting-out, since nice error messages should be the default. Thoughts?

@Nugine Nugine marked this pull request as draft November 16, 2021 04:59
@KodrAus
Copy link
Member

KodrAus commented Nov 16, 2021

A macro-diagnostics feature sounds like a good one to me 👍 Ideally we would have it on by default, but unfortunately for a non-trivial subset of users any additional dependencies will be pushed back against. That's one of our main reasons for a non-proc-macro version, and is also what landed us with a fast-rng feature that pulls in rand. I think there's an alternative approach to default features though that we can take: recommend a set of features to enable at the top of the README and in the crate-level docs:

[dependencies.uuid]
version = "1"
features = [
    "v4",                # allow generating random UUIDs
    "fast-rng",          # use a faster (but still sufficiently random) algorithm
    "macro-diagnostics", # support more detailed diagnostics in the `uuid!` macro
]

It's not on by default, but it's recommended by default.

@Nugine Nugine marked this pull request as ready for review November 16, 2021 06:23
@Nugine Nugine requested a review from KodrAus November 16, 2021 06:23
Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Thanks @Nugine! This looks good to me. I just had one suggestion on a missed feature rename in the docs.

src/lib.rs Outdated Show resolved Hide resolved
@KodrAus KodrAus merged commit 89530db into uuid-rs:main Nov 16, 2021
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.

Macro construction without syn
3 participants