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

Add a new CARGO_PKG_AUTHORS environment variable #2465

Merged
merged 1 commit into from Apr 8, 2016

Conversation

Projects
None yet
4 participants
@TheNeikos
Copy link
Contributor

TheNeikos commented Mar 10, 2016

This will allow crates to use the CARGO_PKG_AUTHORS env variable to get a comma
seperated list of the authors declared in the manifest.

Closes #2441

@TheNeikos

This comment has been minimized.

Copy link
Contributor Author

TheNeikos commented Mar 10, 2016

Something that has been touched upon in #2441 is how to format authors.

I've gone with a join as it keeps the code simple.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 11, 2016

Looks reasonable to me, thanks @TheNeikos!

Curious what others on @rust-lang/tools think as well!

@alexcrichton alexcrichton self-assigned this Mar 11, 2016

@rtaycher rtaycher referenced this pull request Mar 11, 2016

Closed

add crate_authors! macro #447

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 25, 2016

Ok, got a chance to talk about this with @wycats yesterday, and the conclusion is that this seems fine to add, but for lists we probably want to go with colon-separated? Seems less likely to have a colon in the name at least other than a comma.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 27, 2016

@alexcrichton Less likely, but still possible. What about using the standard email format: Some Name some.name@example.org, "Last, First" first.last@example.com That format handles any kind of punctuation, and seems closer to what people expect with a list of names and emails.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 27, 2016

Oh this isn't necessarily for the format itself, moreso for joining two different names together (you can have multiple authors).

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 27, 2016

@alexcrichton That's what I mean. If someone puts this into Cargo.toml:

authors = ["Person One <person.one@example.org>", "Person Two <person.two@example.com>"]

then the environment variable could contain Person One <person.one@example.org>, Person Two <person.two@example.com>, and a macro to return that for programmatic use could either return that as a string or as an array of strings.

If someone decided, for whatever reason, to have an author entry like "\"Person Two, with a comma\" <person.two@example.com>" (which I've seen in real emails, both to write "Last, First" and to write nominal suffixes), then that's still unambiguously parseable.

@TheNeikos

This comment has been minimized.

Copy link
Contributor Author

TheNeikos commented Mar 27, 2016

So, a colon it is then?

authors = ["Person A", "Person, B"]

Would then be: Person A:Person, B ?

Asking to clarify.

This should also work for the concerns @joshtriplett brought up I think?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 28, 2016

@joshtriplett yeah although @wycats was thinking we'd use a colon instead of a comma. We currently don't have a macro to parse this out and it's possible for someone to say something like:

authors = ["foo:bar", "baz"]

where if we use a colon separator this would look like CARGO_PKG_AUTHORS=foo:bar:baz and there's no way to parse that out to the original. We could invent our own quoting syntax with escaping of quotes and the escapes themselves, but without some utility to parse the output it seems like a non-starter unfortunately.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 28, 2016

@alexcrichton How easily can we scan the entire ecosystem of Cargo.toml files and find out whether anyone currently uses a comma or colon in the Authors field? If Cargo bans a character there, then you can use anything you want. Otherwise, you'd want some kind of quoting/escaping, and for that, following rfc5322/rfc2822/rfc822 convention for "mailbox-list" seems like the right approach, along with requiring that each quoted string in the authors list follow the convention for one "mailbox". I feel certain that Rust has at least one compliant parser for that format, and generating it is relatively easy.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 28, 2016

Looks like colons and commas show up unfortunately. Colons show up in some HTTP addresses in the authors field and commas separate either names or multiple email addresses.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 28, 2016

@alexcrichton Odd to see HTTP URLs in the authors field; are people putting things like https://github.com/exampleusername in there? It's unfortunate if the metadata doesn't have any common format.

Regarding comma-separated names and email addresses: isn't the field a list? Are people writing authors=["author 1 <a1@example>, author 2 <a2@example>"] rather than authors=["author 1 <a1@example>", "author 2 <a2@example>"] ? That's definitely incorrect. (There are slightly more legitimate uses of comma inside a single author string, though.)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 28, 2016

Yeah that's basically what I saw from a quick grep, some names look like "last, first " I think.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 28, 2016

@alexcrichton Yeah, some businesses do that even in email headers: "Last, First" first.last@example.com

What about just escaping the names in a consistent way (for instance, always wrap them in single-quotes, and escape single-quotes and backslashes with backslashes), and then putting them in the environment variable separated by anything we like? That's easy enough to reverse back into a list of strings.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 29, 2016

The problem with creating our own syntax for this is then you end up needing to create a parser as well. We've tended to avoid that in the past for things like RUSTFLAGS as well, and I suspect that if we do it here we'd probably want to use the same syntax everywhere.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 30, 2016

☔️ The latest upstream changes (presumably #2531) made this pull request unmergeable. Please resolve the merge conflicts.

@TheNeikos TheNeikos force-pushed the TheNeikos:add-version_env branch from 07233f6 to 2a10d69 Apr 8, 2016

@TheNeikos

This comment has been minimized.

Copy link
Contributor Author

TheNeikos commented Apr 8, 2016

@alexcrichton Is this what was decided? Or should I put in some whitespace as well?

@@ -45,6 +45,7 @@ let version = env!("CARGO_PKG_VERSION");
* `CARGO_PKG_VERSION_MINOR` - The minor version of your package.
* `CARGO_PKG_VERSION_PATCH` - The patch version of your package.
* `CARGO_PKG_VERSION_PRE` - The pre-release version of your package.
* `CARGO_PKG_AUTHORS` - Comma seperated list of authors from the manifest of your package.

This comment has been minimized.

@alexcrichton

alexcrichton Apr 8, 2016

Member

The "comma separated" part here may no longer be right

This comment has been minimized.

@TheNeikos

TheNeikos Apr 8, 2016

Author Contributor

Oops you're right! Changed it in the commit, but forget here. On it.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 8, 2016

Looks good to me! Just a minor nit

Add a new CARGO_PKG_AUTHORS environment variable
This will allow crates to use the CARGO_PKG_AUTHORS env variable to get a colon
seperated list of the authors declared in the manifest.

Closes #2441

@TheNeikos TheNeikos force-pushed the TheNeikos:add-version_env branch from 2a10d69 to dd26ce3 Apr 8, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 8, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 8, 2016

⌛️ Testing commit dd26ce3 with merge 34a7a06...

bors added a commit that referenced this pull request Apr 8, 2016

Auto merge of #2465 - TheNeikos:add-version_env, r=alexcrichton
Add a new CARGO_PKG_AUTHORS environment variable

This will allow crates to use the CARGO_PKG_AUTHORS env variable to get a comma
seperated list of the authors declared in the manifest.

Closes #2441
@bors

This comment has been minimized.

@bors bors merged commit dd26ce3 into rust-lang:master Apr 8, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@TheNeikos TheNeikos deleted the TheNeikos:add-version_env branch Apr 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.