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

allow non-utf8 arguments to proxies #1599

Merged
merged 1 commit into from
Jan 14, 2019
Merged

Conversation

euclio
Copy link
Contributor

@euclio euclio commented Jan 3, 2019

@euclio euclio changed the title allow non-utf8 arguments in proxies allow non-utf8 arguments to proxies Jan 3, 2019
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.

Other than being unsure what 'proxies' are in this context, I think this change looks sensible. Certainly preferable to a panic, though I worry if the user might be surprised if they enter a non-utf8 toolchain name and are then confused when instead of honouring it, it is silently ignored.

} else {
None
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we do the .starts_with('+') before the .to_str() and issue an error if the user has specified a non-utf8 toolchain name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the check and two more tests.

@nrc
Copy link
Member

nrc commented Jan 6, 2019

This looks good, but I agree it would be better to warn the user rather than silently ignore it.

@euclio euclio force-pushed the non-utf8-args branch 6 times, most recently from a46990f to deea97e Compare January 11, 2019 20:35
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.

This seems to do as requested, thank you for the change and the tests.

@nrc nrc merged commit 3f91e93 into rust-lang:master Jan 14, 2019
@nrc
Copy link
Member

nrc commented Jan 14, 2019

Thanks!

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