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 listing of installed targets #1808

Merged
merged 2 commits into from
Apr 27, 2019
Merged

Add listing of installed targets #1808

merged 2 commits into from
Apr 27, 2019

Conversation

onatm
Copy link
Contributor

@onatm onatm commented Apr 25, 2019

This feature adds the ability to list installed targets only.

I'm playing with cross-compilation a lot and I need a way to list only the installed targets.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Hi!

Thanks for this patch, it looks pretty OK but it needs a test and possibly some README updates to complete the work.

Do you think you can see how to write a test for this?

@onatm
Copy link
Contributor Author

onatm commented Apr 25, 2019

Hey!

Thank you for the review. I believe I need to add tests to tests/cli-exact.rs, tests/cli-v2.rs and tests/cli-rustup.rs.

Something similar to:

https://github.com/rust-lang/rustup.rs/blob/7afe6a8b10b4e5f891e1b5232a7c362b40f5c168/tests/cli-exact.rs#L354-L371

Do you think that I am heading to the right direction?

@onatm
Copy link
Contributor Author

onatm commented Apr 25, 2019

I think I managed to implement all the required tests. I am not sure about README updates. Could you point me the place to make documentation changes @kinnison ?

I've got a question about the number of commits: Should I leave as is or squash them into one?

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Hi,

The added tests look excellent, thank you very much!

I'm happy with the commits as they are, or you could squash the three test commits into one.

Either way, I'll look to merge this in soon, unless anyone else objects.

Thanks again,

D.

@onatm
Copy link
Contributor Author

onatm commented Apr 26, 2019

I squashed test commits into one 🎉

I hope this feature will be useful for others as well. Thanks for the review again.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

The squash looks clean

@kinnison kinnison merged commit 701c6f4 into rust-lang:master Apr 27, 2019
@onatm onatm deleted the list-installed-targets branch April 28, 2019 22:32
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.

3 participants