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

Add a rustc version segment with multiple install options #314

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Nov 30, 2021

This adds 4 options for rustc (if installed):

  • By default, will never show the rustc version segment.
  • always will always show the version.
  • yes will show the version of rustc if we can locate the Cargo.toml file (using cargo locate-project).
  • diff will show the version if there's a different result than the global rustc (as ran in the root). This is useful when using .rust-toolchain files to enforce specific version of the toolchain, for example.

@hansl
Copy link
Contributor Author

hansl commented Feb 3, 2022

Show, not just tell:

image

@hansl
Copy link
Contributor Author

hansl commented Feb 3, 2022

@bobthecow This is ready to review when you want, and has been rebased on laster master branch. Thanks!

Copy link
Member

@bobthecow bobthecow left a comment

Choose a reason for hiding this comment

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

How expensive are cargo lookups? Can we default to on if rustc is present?

case yes
cargo locate-project --message-format plain --quiet >/dev/null 2>/dev/null
and set local_rustc_version (__bobthefish_get_cargo_version (__bobthefish_pwd))
case diff
Copy link
Member

Choose a reason for hiding this comment

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

This is the default behavior for most segments (e.g. ruby). Can we make two options, always and yes, and have yes do what diff currently does?

(Alternatively, if it's not to expensive to default on, it would be off/always).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It takes 70msec to run rustc --version. I think it's too much to run with every prompt; if you had 2-3 of these it would be a third of a second and it's noticeable. But I'll defer the final decision to you.

I've done the first part.

functions/__bobthefish_glyphs.fish Outdated Show resolved Hide resolved
@@ -846,6 +847,47 @@ function __bobthefish_prompt_rubies -S -d 'Display current Ruby information'
echo -ns $ruby_glyph $ruby_version ' '
end

function __bobthefish_get_cargo_version -S -d 'Display the cargo version in a directory'
pushd $argv[1]
command rustc --version | cut -d ' ' -f 2
Copy link
Member

Choose a reason for hiding this comment

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

Does rustc take a working directory to save us from a pushd/popd? Otherwise, we should probably add command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no. I'll add command but I'm also exploring if there's a better way to get this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I decided to rely on fish -c " ... " to create a subshell only if we need to (which should be once for the global anyway).

@hansl
Copy link
Contributor Author

hansl commented Feb 9, 2022

How expensive are cargo lookups? Can we default to on if rustc is present?

image

This is a cold run, it stabilizes between 50-70 msecs. Not sure why (file lookup to see if it needs to forward to another rustc, likely).

@hansl
Copy link
Contributor Author

hansl commented Feb 9, 2022

PTAL.

@hansl
Copy link
Contributor Author

hansl commented Aug 30, 2022

Ping, @bobthecow

@hansl hansl requested a review from bobthecow August 30, 2022 23:30
@hansl
Copy link
Contributor Author

hansl commented Nov 4, 2022

@bobthecow Ping. I have additional changes that I would like to submit a PR for, but they're based on this and don't want to pollute your queue.

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.

2 participants