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

Update tooling.md #166

Closed
wants to merge 1 commit into from
Closed

Update tooling.md #166

wants to merge 1 commit into from

Conversation

flip111
Copy link
Contributor

@flip111 flip111 commented Feb 18, 2019

@flip111 flip111 requested a review from a team as a code owner February 18, 2019 19:46
@rust-highfive
Copy link

r? @japaric

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources labels Feb 18, 2019
@flip111
Copy link
Contributor Author

flip111 commented Feb 18, 2019

#143


The text below explains why we are using these tools. Installation instructions
can be found on the next page.

## `cargo-generate` OR `git`

## Using a template for the first project ##
Copy link
Member

Choose a reason for hiding this comment

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

All headings are named after tools. With this change that would no longer be the case. I would prefer to keep the symmetry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about cargo-generate and other tools ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, maybe write something about rustc too ..

Copy link
Member

Choose a reason for hiding this comment

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

The current text matches what's listed in the bullet list. The "other tools" in your proposal makes it unclear whether "other tools" means just git or all / some of the other tools in the bullet list.

Personally, I think the current text is fine.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, maybe write something about rustc too ..

Rust developers (the target audience of this book) are familiar with rustc / Cargo; I don't think it's necessary to add a section for them.

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 just find it strange that the choice now is between cargo generate and git both tools need to be downloaded and installed, while the other tools "curl, wget, or your web browser." are likely to be already on a system. But i'm tired to bike shed over this, so you can get the final call, i'll leave it as it is or make it whatever.

- [OPTIONAL] `git` OR
[`cargo-generate`](https://github.com/ashleygwilliams/cargo-generate). If you
have neither installed then don't worry about installing either.
- [`cargo-generate`](https://github.com/ashleygwilliams/cargo-generate) and `git`.
Copy link
Member

Choose a reason for hiding this comment

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

This seems fine to me

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this should be "(..) or git". You don't need both tools; one of them suffices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Later on in the book cargo generate --git command is used. We can put or here when comes with it's own git api and doesn't rely on standalone git. I assumed it relies on standalone git.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From this wording i conclude that standalone git is not required https://github.com/rust-lang/git2-rs#version-of-libgit2 (dependency of cargo generate)

Copy link
Member

Choose a reason for hiding this comment

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

cargo-generate doesn't shell out to git; it uses libgit.

@japaric
Copy link
Member

japaric commented Feb 18, 2019

@flip111, in that case let's leave the text as it is. Could you please close the two issues you linked from here? Thanks.

@japaric japaric closed this Feb 18, 2019
@flip111
Copy link
Contributor Author

flip111 commented Feb 18, 2019

I was referring to line 24 which you commented on. Since this PR is now closed i opened a new PR with only the pieces that didn't get negative feedback. #172 I will close the second issue later

@andre-richter andre-richter mentioned this pull request Feb 18, 2019
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. T-resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants