feat: implement timer module #118
Conversation
|
A couple of notes about this: first, I've utterly goobered the git stuff--it's showing 25 commits to merge in, some of which I'm pretty sure were already merged. If anyone knows how to fix this, please let me know. Second, I'm not sure if the design of the rendering function is sane--I think it's nice, but it also has a Finally, there are two places where the things can silently fail:
|
This has happened to me more times than I can count. 😅 |
yes @chipbuster @matchai ... this happens when you're on a branch (not the master branch) that you have previously opened a PR for (that is now merged) ... and then you create a new branch off of this non-master branch so your new branch has all of your old commits (which have now been merged into master), and all of your new commits |
|
@johnletey Thanks, that explains it. I think I accidentally merged starship/master into my branch, instead of rebasing my branch onto it, and they diverged some time ago. If we don't squash on merge, do you know how the process is handled? Will Github detect that some of the commits are duplicate and skip over them? |
|
If we don't squash we'll get repeat commits. 🤷♂️ |
Oh boy ... not quite sure ... that's as far as my Github expertise goes! |
Assuming you have at least one branch that is up-to-date with starship/master, you can rebase against that one. I like to setup an upstream remote for that, then you should be able to do something like: git rebase -i upstream/masterWith this git should already try and deduplicate commits, and you will be given the option to |
|
I totally forgot that we also need to add some integration tests and documentation for this new module. The following would need doing:
|
|
I'd be happy to do it, if you're willing to deal with a couple more reviews. I'm also starting to think I should hold off on working on this until this conference is over (Friday) and I have a chance to make up some sleep--I'm made some awfully silly mistakes the last few days. What would need to be added for integration tests? Would it just be a bunch of tests using |
|
Haha, no rush. I don't mind going through some more reviews. Enjoy your conference and we can take care of this later. 😄
Yep, that should be fine. This is what I was thinking:
The recent |
This adds support for qualified paths (e.g. using `/usr/local/bin/zsh` instead of `zsh`) to init.rs. The function now converts the shell name into an OsStr, then to a Path, then gets the file stem, and unwraps back into a str. While this process can fail (yielding a None), it's highly unlikely to unless the user has messed with their shells or there's an issue in Starship--therefore, the failure message in this case simply asks the user to file a bug report.
In the bash/zsh init scripts, PS1/PROMPT are being evaluated once (when the variable is assigned to) because double quotes do not sufficiently defer command substitution. This leads to the prompt getting "stuck" wherever starship was initialized. This changes the init script to use single quotes, deferring command substitution until the prompt variable is evaluated.
Add support for paths to init function This adds support for qualified paths (e.g. using `/usr/local/bin/zsh` instead of `zsh`) to init.rs. The function now converts the shell name into an OsStr, then to a Path, then gets the file stem, and unwraps back into a str. While this process can fail (yielding a None), it's highly unlikely to unless the user has messed with their shells or there's an issue in Starship--therefore, the failure message in this case simply asks the user to file a bug report.
Making use of PROMPT_COMMAND in bash and precmd in zsh, the prompt is no longer being expanded and rendered when the variable is initially set.
Add all necessary code for starship to take in time elapsed since command execution started with flag --elapsed. It is now possible to run e.g. `starship prompt --status=0 --elapsed=100` and get a timer print.
Add support for all three shells into init.rs. This modifies the shell init scripts quite heavily.
The option min_time in section [timer] will affect the minimum time for when the shell displays job time. The default value is 2.
Caused by implementing the timer module.
Co-Authored-By: Matan Kushner <hello@matchai.me>
|
@all-contributors please add @chipbuster for docs and test. |
|
I've put up a pull request to add @chipbuster! 🎉 |
This PR adds a module in starship that adds a job timer.
Description
Implement a timer module that takes a commandline argument, the number of seconds the last job took to complete, and displays it if appropriate.
Alters shell initialization files to compute this number using
date +%swhere needed.Adds a config section to configure minimum amount of time before timer is shown (default is 2s)
Motivation and Context
Closes #104
Types of changes
Screenshots (if appropriate):
Top-to-bottom: bash, zsh, fish (zsh is slightly doctored because my old shell config kept throwing some lines in there)
How Has This Been Tested?
Tested by using in all three environments. Bash and Zsh proved surprisingly tricky to do correctly, so their shell configs are now fairly large.
Behavior is not 100% consistent between shells: when spamming empty commands, fish will continue to display the last command time, while bash and zsh will not.
Passes all integration tests.
Checklist:
(But only in-module tests--don't know if there are tests elsewhere)