feat(rust): Option to display toolchain names by qryxip #3414

chipbuster · 2022-01-11T05:13:30Z

Description

#559 was approved and then sat around waiting to be merged for over 18 months. I'm not sure what happened, but I want to extend my sincerest apologies to for @qryxip for the state of that PR. We clearly should have merged it before things got this bad.

I attempted to rebase this PR onto master several times, but to no avail: the PR comes from before the format strings change, so there were some incompatibilities with how it was structured.

This PR is an attempt to make things right: I have manually ported over the changes made by @qryxip in #559. Because they would have potentially constituted breaking changes (I'm not sure what we consider breaking right now), I have segmented off two code paths. The old codepath uses exactly the same code as is present on master. The new code path, based on #559, exposes two new format variables, named numver and toolchain. Enabling the behavior intended by #559 involve simply setting format = "via [$symbol($numver)\$($toolchain)\$ ]($style)" in starship.toml.

Because so many codepaths potentially create performance issues, I have also created a module-level external command cache, known as RustToolingEnvironmentInfo. This uses OnceCell to cache the results of reading data from the environment. As a result, this version of the module is about twice as fast as master, though I have had not had time to do thorough benchmarks.

Motivation and Context

Closes #559
Closes #555

Screenshots (if appropriate):

I can replicate the behavior as shown in this comment on #559

TODOs

The tests are still broken due to the removal of certain override extractions that I have not had time to patch up.
Whitespace handling is not perfect and needs to be tuned.
~~Add styling options for $numver and $toolchain.~~ (REJECTED)
Update documentation
Need to add tests to cover new codepaths.

Questions Moving Forward

I have these questions for the rest of Starship Command:

Is this idea (in the abstract) still something we're still interested in merging? If no, ignore the remaining questions.
Is the current path an acceptable way forward? This lets us incorporate the new features and keep the old ones with better performance, but the module has grown from ~580 lines of code to 800+, and I haven't added any new tests yet. If the code bloat is unacceptable, there may be other ways to merge this.
How do we make sure @qryxip get the proper credit for this? I am willing to go as far as force-pushing this branch to their repository to revive feat(rust): Option to display toolchain names #559 if we feel it is an appropriate mechanism.

davidkna

Didn't look at this in depth yet, but I have a few comments

src/modules/rust.rs

segevfiner · 2022-01-20T07:30:06Z

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

chipbuster · 2022-01-25T21:56:35Z

Rebased with squash to reduce the commit set (and strip out a few commits that were made in pure confusion)

davidkna

Beyond my comment, the changes look reasonable, but I might have missed something.

src/modules/rust.rs

chipbuster · 2022-02-16T16:11:42Z

Okay. I'll let this sit another few days and then try to do a self-review, since I think it's been long enough that I can come at it with a (relatively) clean slate.

davidkna · 2022-03-08T17:35:25Z

src/modules/rust.rs

+    fn lookup_override(&self, cwd: &Path) -> Option<String> {
+        self.overrides
+            .iter()
+            .find(|(dir, _)| cwd.starts_with(dir))
+            .map(|(_, name)| name.to_owned())
+    }


This will need to integrate the changes from #3359 (longest match) and #3668 (proper prefix handling).

Milo123459 · 2022-03-10T21:34:20Z

Quick comment: nightly toolchains are suffixed with -nightly. It would be nice if this module doesn't display "nightly" twice (I presume it currently does).

matchai · 2022-03-10T22:46:37Z

Quick comment: nightly toolchains are suffixed with -nightly. It would be nice if this module doesn't display "nightly" twice (I presume it currently does).

If you take a look at the screenshot in the issue description, you can see the proposed output for nightly toolchains. "nightly" is only shown once.

Milo123459 · 2022-03-10T22:49:11Z

Ah, nice. This module looks really good by the way!

davidkna · 2022-03-10T22:58:47Z

I think numver doesn't include the toolchain name for easier use with toolchain.
The default format string seems to be unchanged and should behave mostly the same.

matchai · 2022-03-10T23:37:07Z

Ah, I was of the impression that we were changing the default config for the Rust module. Probably something we should clear up. My thinking is that the updated format shouldn't constitute a breaking change because the same data is being presented in an easier-to-read format.

Imo, breaking the semantic meaning of the prompt should be breaking (e.g. changing a symbol), or changing what information can be presented.

Nevertheless, we do have breaking changes lined up for the next major bump. Perhaps we can update the config group this change as part of them, just to be safe?

How do you feel about changing the default format?

matchai · 2022-04-04T15:59:27Z

If we can agree on whether this should be released as a breaking or non-breaking change, I think it's ready to be merged.

I'd personally vote to make it non-breaking:
It's an enhancement of the default prompt without a semantic change. Custom format strings are unaffected.

davidkna · 2022-04-04T18:03:01Z

It might be nice to handle this first before merging, but that could also be done later.
I would also consider this a non-breaking change.

andytom · 2022-04-04T21:16:35Z

I think this should be fine to treat as a non breaking change.

chipbuster · 2022-04-05T17:40:15Z

Okay. I'll need to pull in the changes as mentioned in this comment, and then this will probably be ready for final review.

EDIT: And also this one

davidkna

Wait, I just noticed toolchain resolves to stable-aarch64-apple-darwin on my mac, which is too long and doesn't match the documentation.

chipbuster · 2022-05-04T19:53:52Z

Wait, I just noticed toolchain resolves to stable-aarch64-apple-darwin on my mac, which is too long and doesn't match the documentation.

I've figured out why this happens: the existing code, along with all tests, assumes that the default_host_triple value is set in the rustup settings file. This is not necessarily correct, and when it's not set, we currently just assume that there's no host target triple.

I've pulled in guess_host_triple (which was originally derived from rustup's code) to deal with this, though I don't know how to add a test for it since host triples are inherently part of the execution environment.

EDIT: I think long-term, the solution will be to move these methods into the RustupSettings struct, then use something like mockall to mock the values.

davidkna · 2022-05-04T20:19:04Z

src/modules/rust.rs

+    let default_host_triple = rust_env_info
+        .get_rustup_settings(context)
+        .default_host_triple()
+        .or_else(|| guess_host_triple());


Suggested change

.or_else(|| guess_host_triple());

.or_else(guess_host_triple);

Somehow, there's a lifetime error that happens when you do that. I don't have the motivation to debug lifetime issues on chained methods at the moment, which is why I have this weird set of assignments in the current commit, but that's definitely something to tackle later.

This is an actualization of PR starship#559 as originally envisioned by qryxip. Adds the ability to display toolchain versions, either as extracted from environment/settings files or by getting the host triple. As part of this, several other major changes were needed: - Many of the smaller functions within the code have been fused, moved, or dropped. - The Rustup environmental info is now initialized lazily using OnceCells. This will hopefully lead to a performance increase. - New configuration variables (`toolchain` and `numver`) have been added to allow finer-grained configuration. - Override information is no longer read from `rustup` output. Instead, it is parsed from the same files that rustup would use to determine this info. Co-authored-by: qryxip <qryxip@gmail.com>

davidkna

By the way, I don't think you added commit credit for @qryxip yet. If you had trouble figuring out emails/the syntax, the GitHub desktop application works quite nicely for this.

chipbuster · 2022-05-04T20:23:40Z

This is what I see from the commits page. Do I need to look somewhere else to see if credit counts on the commit?

davidkna · 2022-05-04T20:25:12Z

This is what I see from the commits page. Do I need to look somewhere else to see if credit counts on the commit?

No it looks fine, I must have missed it.

chipbuster · 2022-05-04T20:42:44Z

@davidkna Thanks so much for putting time into reviewing this and catching my mistakes on multiple occasions!

Same to @matchai, @andytom, and @segevfiner for commenting, and of course, a huge thanks to @qryxip for proposing and writing most of the logic for this in the first place.

Now to deal with the aftermath, I have some things I'd like to fix in #3967. Work is never done :)

chipbuster requested review from andytom, jletey and matchai January 11, 2022 05:13

davidkna reviewed Jan 13, 2022

View reviewed changes

src/modules/rust.rs Outdated Show resolved Hide resolved

src/modules/rust.rs Outdated Show resolved Hide resolved

src/modules/rust.rs Outdated Show resolved Hide resolved

chipbuster force-pushed the toolchain-name-merge branch 3 times, most recently from 3e37d23 to 11bc895 Compare January 25, 2022 21:55

chipbuster mentioned this pull request Jan 31, 2022

feat!: Use pure symbols, emoji and Powerline glyphs only by default #3544

Open

5 tasks

chipbuster force-pushed the toolchain-name-merge branch from 11bc895 to 6fce47f Compare February 2, 2022 20:12

davidkna reviewed Feb 12, 2022

View reviewed changes

src/modules/rust.rs Outdated Show resolved Hide resolved

chipbuster force-pushed the toolchain-name-merge branch from 6fce47f to a9fe0ec Compare February 15, 2022 03:13

davidkna mentioned this pull request Mar 6, 2022

fix(rust): overrides should only check full segments #3668

Merged

5 tasks

davidkna reviewed Mar 8, 2022

View reviewed changes

matchai mentioned this pull request Apr 7, 2022

feat(rust): Option to display toolchain names #559

Closed

8 tasks

chipbuster force-pushed the toolchain-name-merge branch from 523f493 to a9819d1 Compare May 3, 2022 03:01

chipbuster mentioned this pull request May 4, 2022

fix(rust): fix overrides on windows and set cwd #3359

Merged

5 tasks

chipbuster force-pushed the toolchain-name-merge branch 2 times, most recently from a244e4d to 1837486 Compare May 4, 2022 03:26

davidkna requested changes May 4, 2022

View reviewed changes

chipbuster force-pushed the toolchain-name-merge branch 2 times, most recently from 14dea80 to 5162e4d Compare May 4, 2022 20:01

chipbuster requested a review from davidkna May 4, 2022 20:03

davidkna reviewed May 4, 2022

View reviewed changes

chipbuster force-pushed the toolchain-name-merge branch from 5162e4d to 87802e4 Compare May 4, 2022 20:20

davidkna approved these changes May 4, 2022

View reviewed changes

chipbuster merged commit 393d62c into starship:master May 4, 2022

chipbuster deleted the toolchain-name-merge branch May 4, 2022 20:41

github-actions bot mentioned this pull request May 4, 2022

chore(master): release 1.7.0 #3942

Closed

chipbuster mentioned this pull request May 4, 2022

Code Quality Improvements for the Rust module #3967

Open

github-actions bot mentioned this pull request May 18, 2022

chore(master): release 1.7.0 #3994

Merged

This was referenced Jun 26, 2022

chore(master): release 1.10.0 #4118

Closed

chore(master): release 1.9.1 #4119

Merged

chore(master): release 1.10.0 #4123

Merged

This was referenced Aug 14, 2022

chore(master): release 1.10.0 #4249

Merged

chore(master): release 1.10.0 #4250

Closed

chore(master): release 1.10.0 #4261

Closed

chore(master): release 1.10.0 #4283

Closed

github-actions bot mentioned this pull request Sep 8, 2022

chore(master): release 1.11.0 #4353

Merged

github-actions bot mentioned this pull request Oct 15, 2022

chore(master): release 1.12.0 #4485

Merged

github-actions bot mentioned this pull request Dec 13, 2022

chore(master): release 1.10.0 #4714

Closed

This was referenced Feb 25, 2023

chore(master): release 1.10.0 #4933

Closed

chore(master): release 1.10.0 #4934

Closed

chore(master): release 1.10.0 #4938

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust): Option to display toolchain names by qryxip #3414

feat(rust): Option to display toolchain names by qryxip #3414

chipbuster commented Jan 11, 2022 •

edited by andytom

Loading

davidkna left a comment

segevfiner commented Jan 20, 2022

chipbuster commented Jan 25, 2022

davidkna left a comment

chipbuster commented Feb 16, 2022

davidkna Mar 8, 2022

Milo123459 commented Mar 10, 2022

matchai commented Mar 10, 2022

Milo123459 commented Mar 10, 2022

davidkna commented Mar 10, 2022

matchai commented Mar 10, 2022 •

edited

Loading

matchai commented Apr 4, 2022

davidkna commented Apr 4, 2022

andytom commented Apr 4, 2022

chipbuster commented Apr 5, 2022 •

edited

Loading

davidkna left a comment •

edited

Loading

chipbuster commented May 4, 2022 •

edited

Loading

davidkna May 4, 2022

chipbuster May 4, 2022

davidkna left a comment

chipbuster commented May 4, 2022

davidkna commented May 4, 2022

chipbuster commented May 4, 2022

	.or_else(\|\| guess_host_triple());
	.or_else(guess_host_triple);

feat(rust): Option to display toolchain names by qryxip #3414

feat(rust): Option to display toolchain names by qryxip #3414

Conversation

chipbuster commented Jan 11, 2022 • edited by andytom Loading

Description

Motivation and Context

Screenshots (if appropriate):

TODOs

Questions Moving Forward

davidkna left a comment

Choose a reason for hiding this comment

segevfiner commented Jan 20, 2022

chipbuster commented Jan 25, 2022

davidkna left a comment

Choose a reason for hiding this comment

chipbuster commented Feb 16, 2022

davidkna Mar 8, 2022

Choose a reason for hiding this comment

Milo123459 commented Mar 10, 2022

matchai commented Mar 10, 2022

Milo123459 commented Mar 10, 2022

davidkna commented Mar 10, 2022

matchai commented Mar 10, 2022 • edited Loading

matchai commented Apr 4, 2022

davidkna commented Apr 4, 2022

andytom commented Apr 4, 2022

chipbuster commented Apr 5, 2022 • edited Loading

davidkna left a comment • edited Loading

Choose a reason for hiding this comment

chipbuster commented May 4, 2022 • edited Loading

davidkna May 4, 2022

Choose a reason for hiding this comment

chipbuster May 4, 2022

Choose a reason for hiding this comment

davidkna left a comment

Choose a reason for hiding this comment

chipbuster commented May 4, 2022

davidkna commented May 4, 2022

chipbuster commented May 4, 2022

chipbuster commented Jan 11, 2022 •

edited by andytom

Loading

matchai commented Mar 10, 2022 •

edited

Loading

chipbuster commented Apr 5, 2022 •

edited

Loading

davidkna left a comment •

edited

Loading

chipbuster commented May 4, 2022 •

edited

Loading