Revamp command duration helper/plugin #1906
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaelicWizard
force-pushed
the
command_duration
branch
3 times, most recently
from
a70f425
to
3a8c78e
Compare
|
I've just discovered cmd-returned-notify.plugin and now I'm confused why there's two of these...? |
So, |
NoahGorny
requested changes
Aug 3, 2021
There was a problem hiding this comment.
I have left a comment. Also, for some reason, this patch does not for me, the preexec function seems to not execute.. Does this work on your end @gaelicWizard ?
gaelicWizard
force-pushed
the
command_duration
branch
3 times, most recently
from
475c56f
to
2a1aceb
Compare
gaelicWizard
force-pushed
the
command_duration
branch
from
2a1aceb
to
d8669e2
Compare
|
I'm marking this draft b/c I think I'm going to try to just merge with the plugin. Use one function to store the start time, then let the plugin and the theme separately use the same variable for (a) beep, or (b) prompt display. |
gaelicWizard
force-pushed
the
command_duration
branch
2 times, most recently
from
6ba524b
to
0e38baf
Compare
gaelicWizard
force-pushed
the
command_duration
branch
9 times, most recently
from
21fcd63
to
cf6ef65
Compare
|
I've had to rebase on my |
gaelicWizard
force-pushed
the
command_duration
branch
4 times, most recently
from
569cfeb
to
92175b6
Compare
davidpfarrell
changed the title
gaelicWizard
force-pushed
the
command_duration
branch
from
92175b6
to
6de01a9
Compare
gaelicWizard
force-pushed
the
command_duration
branch
2 times, most recently
from
5da7bcd
to
fa5aabe
Compare
NoahGorny
reviewed
Jan 29, 2022
gaelicWizard
force-pushed
the
command_duration
branch
from
fa5aabe
to
0427758
Compare
gaelicWizard
force-pushed
the
command_duration
branch
2 times, most recently
from
c4939a3
to
3123f94
Compare
|
Rebased on current master |
gaelicWizard
force-pushed
the
command_duration
branch
2 times, most recently
from
9726068
to
3e3bd4a
Compare
|
Rebased & ready! |
gaelicWizard
force-pushed
the
command_duration
branch
2 times, most recently
from
401e032
to
161a9eb
Compare
gaelicWizard
force-pushed
the
command_duration
branch
2 times, most recently
from
1c3f5c5
to
71c5a75
Compare
|
BATS error should be fixed by #2105. |
Calculate the position (from 1 to 12) of the hour hand on the clock emoji used for the _command_duration string. Expressly handle COMMAND_DURATION_COLOR as blank when undefined.
gaelicWizard
force-pushed
the
command_duration
branch
from
71c5a75
to
0a81577
Compare
Fallback to `$SECONDS` for older versions of _Bash_. Instead of shortcircuiting the definition, just short-circuit the function. This allows the variable to be set later, e.g. on theme change.
Use `$EPOCHREALTIME` (or `$SECONDS`) built-in variable provided by Bash instead of `date +%s`. We're only measuing the difference in seconds, so avoid both the binary invocation as well as the subshell. Alsö, Reduce environmental pollution by not exporting every variable, and unsetting when done. Change variable names to match lib/command-duration Remove `preexec_return_notification()` in favor of `lib/command-duration`'s `_command_duration_pre_exec()`. This should now use the same preexec hook and variables as the theme library `command_duration`. tests: handle nanoseconds
Rename the `theme/command_duration.theme` file as it's not really got anything to do with theming or SCM.
gaelicWizard
force-pushed
the
command_duration
branch
from
0a81577
to
866e5be
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Combine
themes/command_duration.theme.bashandplugins/cmd-returned-notify.plugin.bash, and rewrite them to use$EPOCHSECONDS(or$SECONDSon older Bash) while eliminating temporary files on disk.Motivation and Context
Removing the temp file reduces complexity (no need to locate $TMPDIR, read file back), reduces bug surface (what happens if /tmp cleaner runs before prompt returns?), and improves performance (no need to touch the disk!). Adopting
$EPOCHSECONDSremoves entirely the need for subshells or externaldateinvocation, improving performance and reducing points of failure. Sharing the preexec hook reduces total hooks running, and reduces duplicate code.And, of course,
command_durationdidn't work at all on Mac OS X since BSDdatedoesn't support%1N; now it works perfectly on both Mac and Linux (and BSD!) and doesn't usedateat all.How Has This Been Tested?
This change is live in my branch on my system now.
Types of changes
Checklist:
clean_files.txtand formatted it usinglint_clean_files.sh.