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

docs: update default edition value #9228

Closed
wants to merge 1 commit into from

Conversation

gokaygurcan
Copy link

Hi 👋 ,

What's changing

Looks like the default edition value has changed from 2015 to 2018 along the way. Here, we're making sure the docs are showing the correct value.

Way to reproduce/confirm

(considering you have Docker installed)

# run a new Ubuntu container
docker run -it --rm ubuntu /bin/bash

# install curl
apt-get update -qq && apt-get install -yqq curl

# install Rust and Cargo
curl https://sh.rustup.rs -sSf | sh
source $HOME/.cargo/env

# create a new folder and initialise a new project
mkdir edition
cd edition/
cargo init

# check the edition value in newly created Cargo.toml
cat Cargo.toml 

The output should be something like

[package]
name = "edition"
version = "0.1.0"
authors = []
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

I hope this makes sense and helps a bit.

Best,
G.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2021
@Eh2406
Copy link
Contributor

Eh2406 commented Mar 2, 2021

#9202 (comment)

@gokaygurcan
Copy link
Author

gokaygurcan commented Mar 2, 2021

Hi @Eh2406,

Yes, I had the same confusion. Then tried it again couple of times, with and without a fresh environment. And I thought it'd be nice to correct that value. This is defn. not a long term solution though.

Feel free to close the PR if you think this is not needed.

@alexcrichton
Copy link
Member

@gokaygurcan out of curiosity, how did you come across this documentation and what led to your desire to fix it? I imagine there was confusion after reading the documentation, but I'm curious how you got on this page and what led you to it in the first place.

@gokaygurcan
Copy link
Author

Hi @alexcrichton,

Don't know why that matters but here we go:

I was starting with a new project. My plan was to use Rust for some low level operations and stuff, and expose some methods through JavaScript and in the end publish the project as an npm package. I initially started with Neon, then thought I should also give it a try without any dependency. And after running cargo init and checking Cargo.toml, I saw a message saying

See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

Since edition is one of the default fields in Cargo.toml, I checked what are the options and what should I see in there. And saw that the values were not matching. Tried couple of more times with and without Docker (follow the example above), and confirmed that it's not right in the docs, hence, this PR.

I'm not sure how many people opens that link in Cargo.toml to see other fields or possible values for each field, but I think this is a very confusing situation for any beginner.

So, to answer your question, it's somewhat impossible to not come across with that documentation, and I think this is one of the key reasons of doing an open source project: people can contribute. I saw an error, tried to make it right.

Anyway, all in all, the ball is in your court. Feel free to ask more questions, or explanations, or back story, the incorrect value stays online while we're talking here.

Best,
G.

@alexcrichton
Copy link
Member

Ok thanks for the info, that's what I'd assumed but it was good to make sure.

To clarify, the wording in the stable documentation and the recently revised wording (not currently published to the nightly channel just yet) are both correct. The default value for this key is 2015 if it is not specified. The key is, however, filled in with cargo new by default to 2018. This means that this PR's update to the documentation is not correct.

I've tried to reword this though to clear things up in #9233. Could you take a look at that and see if it dispels the perceived ambiguity here?

@gokaygurcan
Copy link
Author

Hi again,

Yea, that's good I guess. At least, now I can understand I should expect some value, and there's also a default value to support backwards compatibility.

Thanks for the update. I think we don't need this PR anymore.

@gokaygurcan gokaygurcan closed this Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants