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

Migrate browser opening logic to match Cargo #1830

Merged
merged 1 commit into from May 5, 2019

Conversation

LPGhatguy
Copy link
Contributor

May impact #1125.

Cargo uses the opener crate for opening the user's browser. I noticed some interest in deduplicating and unifying the logic used between Rustup and Cargo in a couple places, and I think this is a reasonable part of doing that.

This PR removes about 80 lines of platform-specific code with a dependency on opener, which has no dependencies of its own. It indirectly removes an outstanding FIXME in the codebase.

@kinnison
Copy link
Contributor

kinnison commented May 3, 2019

Hi,

Thank you for your contribution. Unfortunately the CI does not pass because your code is not formatted cleanly.

Could you please rebase your change to fold the crate version commit into your main commit, and also ensure that cargo fmt has been run so that the formatting is clean.

Once you've done that and pushed your branch once more, I shall review it again.

Fundamentally the change looks good, but I cannot approve it until you have done this.

Thanks again,

D.

@LPGhatguy
Copy link
Contributor Author

I formatted my change with rustfmt and went ahead and rebased + squished the commits.

@LPGhatguy LPGhatguy closed this May 5, 2019
@LPGhatguy LPGhatguy reopened this May 5, 2019
@LPGhatguy
Copy link
Contributor Author

Looks like checks are green now (I had to open/close to rerun them because of a failure installing Rust on the CI machine initially).

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.

LGTM, and this should be easy to back out if it proves not to work, though given cargo uses this crate it seems likely to be fine.

@kinnison kinnison merged commit cdcac43 into rust-lang:master May 5, 2019
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.

None yet

2 participants