Uh oh!
There was an error while loading. Please reload this page.
Fix suggestions for x.py setup - #77400
Conversation
rust-highfive
commented
Oct 1, 2020
(rust_highfive has picked a reviewer for you, use r? to override) |
jyn514
commented
Oct 1, 2020
r? @jyn514 |
jyn514
commented
Oct 1, 2020
That seems like it would be useful, yeah. I thought of doing it in the original PR but never got around to it. Maybe something like this? enumProfile{Compiler,Codegen,Library,User,}// keeps case-insensitive matching, etc.implFromStrforProfile{ ... }implDisplayforProfile{ ... }implProfile{fninclude_path(&self,src_path:&Path) -> PathBuf} |
Uh oh!
There was an error while loading. Please reload this page.
jyn514
commented
Oct 5, 2020
@alarsyo are you still interested in making the more general change I mentioned in #77400 (comment)? No problem if not, this is good to merge as-is. |
alarsyo
commented
Oct 5, 2020
I am! Pushing a patch soon |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
I'm not happy with having the a, b, c, d letters from the prompt as part of the FromStr implementation, maybe having a match like this
match choice_string {"a" => Profile::Library,"b" => Profile::Compiler,"c" => Profile::Codegen,"d" => Profile::User,
other => match other.parse(){Ok(profile) => profile,Err(prof_err) => { print error,continue;}}}in the prompting function would be better
There was a problem hiding this comment.
I don't think it's a big deal either way. I kind of like it the way it is since it means there's only one place the match has to be exhaustive, but you could convince me it would be easier to keep a/b/c in sync if it's next to the prompt.
There was a problem hiding this comment.
You're right, having only one match to keep exhaustive might be best :)
Uh oh!
There was an error while loading. Please reload this page.
jyn514
left a comment
There was a problem hiding this comment.
This already looks a lot better :)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
I don't think it's a big deal either way. I kind of like it the way it is since it means there's only one place the match has to be exhaustive, but you could convince me it would be easier to keep a/b/c in sync if it's next to the prompt.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f091639 to
c09cf00Comparejyn514
commented
Oct 5, 2020
@bors r+ Thanks for working on this! |
bors
commented
Oct 5, 2020
📌 Commit c09cf00d782bc16e795bf278eb67ee017240cbe6 has been approved by |
bors
commented
Oct 6, 2020
☔ The latest upstream changes (presumably #77606) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
c09cf00 to
45d17e2Comparealarsyo
commented
Oct 6, 2020
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
45d17e2 to
75e464eCompare75e464e to
d67a7e6Comparejyn514
commented
Oct 6, 2020
@bors r+ |
bors
commented
Oct 6, 2020
📌 Commit d67a7e6 has been approved by |
Rollup of 11 pull requests Successful merges: - rust-lang#76784 (Add some docs to rustdoc::clean::inline and def_id functions) - rust-lang#76911 (fix VecDeque::iter_mut aliasing issues) - rust-lang#77400 (Fix suggestions for x.py setup) - rust-lang#77515 (Update to chalk 0.31) - rust-lang#77568 (inliner: use caller param_env) - rust-lang#77571 (Use matches! for core::char methods) - rust-lang#77582 (Move `EarlyOtherwiseBranch` to mir-opt-level 2) - rust-lang#77590 (Update RLS and Rustfmt) - rust-lang#77605 (Fix rustc_def_path to show the full path and not the trimmed one) - rust-lang#77614 (Let backends access span information) - rust-lang#77624 (Add c as a shorthand check alternative for new options rust-lang#77603) Failed merges: r? `@ghost`
#76631 introduced a new
setupcommand to x.pyBy default the command prompts for a profile to use:
and then displays command suggestions, depending on which profile was chosen. However the mapping between chosen profile and suggestion isn't exact, leading to suggestions not being shown if the user presses
cord. (because "c" is translated to "llvm" and "d" to "maintainer", but suggestions trigger for "codegen" and "user" respectively)A more thorough refactor would stop using "strings-as-type" to make sure this kind of error doesn't happen, but it may be overkill for that kind of "script" program?
Tagging the setup command author: @jyn514