-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix Rust installation, as it's expecting confirmation (typing 'y') to… #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing that, LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
=======================================
Coverage 68.89% 68.89%
=======================================
Files 14 14
Lines 2887 2887
Branches 11 11
=======================================
Hits 1989 1989
Misses 898 898 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not deviate from the official rust installation instructions. Skipping the installation prompts risks the user being unaware of what is being installed and if the rust installation script detects problems and presents warnings these might be ignored. This might also ignore important prompts added in the future as well.
That said, it is perfectly fine to adjust the explanatory text to mention that the user should follow the prompts and/or to add a hint (in text) that installation can be automated using sh -s -- -y
, but we should not present that as the default.
We already provide a link to the Rust installation instructions right above these code lines. Based on Joe's comment, I'm going to go ahead and close this pull request. |
@simonmarty and @joebaro I understand your point.
as indicated in the instructions, the ". "HOME..." will be pasted instead of the 'y'. |
Clarified it in #27. Maybe we should just delete that code block entirely and just point users to the Rust installation documentation to reduce confusion if these instructions change in the future. |
Adding a |
*Issue #, if available:* *Description of changes:* Follow up on convo in #26 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
… proceed
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.