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

Allow for longer command min duration #2198

Merged
merged 1 commit into from
Feb 22, 2023
Merged

Allow for longer command min duration #2198

merged 1 commit into from
Feb 22, 2023

Conversation

OMEGARAZER
Copy link
Contributor

Description

Allows for setting "COMMAND_DURATION_MIN_SECONDS" to more than 60 seconds without adding to the prompt. Previously always showed with lengths over 60 seconds regardless of setting. The documentation does not mention that it would always show when over 1 minute so no change is necessary.

Motivation and Context

Allows setting minimum duration for prompts longer than 60 seconds.

How Has This Been Tested?

Tested locally on Ubuntu 22.04 and Debian 11 using sleep of various lengths.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

Great Catch ! I suggest a slightly different fix however:

before

	if ((command_duration > 0)); then
		minutes=$((command_duration / 60))
		seconds=$((command_duration % 60))
	fi

	_dynamic_clock_icon "${command_duration}"
	if ((minutes > 0)) && ((command_duration >= COMMAND_DURATION_MIN_SECONDS)); then
		printf "%s %s%dm %ds" "${COMMAND_DURATION_ICON:-}" "${COMMAND_DURATION_COLOR:-}" "$minutes" "$seconds"
	elif ((seconds >= COMMAND_DURATION_MIN_SECONDS)); then
		printf "%s %s%d.%01ds" "${COMMAND_DURATION_ICON:-}" "${COMMAND_DURATION_COLOR:-}" "$seconds" "$deciseconds"
	fi

after

	if ((command_duration >= COMMAND_DURATION_MIN_SECONDS)); then
		minutes=$((command_duration / 60))
		seconds=$((command_duration % 60))

		_dynamic_clock_icon "${command_duration}"
		if ((minutes > 0)); then
			printf "%s %s%dm %ds" "${COMMAND_DURATION_ICON:-}" "${COMMAND_DURATION_COLOR:-}" "$minutes" "$seconds"
		else
			printf "%s %s%d.%01ds" "${COMMAND_DURATION_ICON:-}" "${COMMAND_DURATION_COLOR:-}" "$seconds" "$deciseconds"
		fi
	fi

Essentially bail earlier if the threshold isn't met - It also looks a bit cleaner.

Lemme know what you think and thanks for putting this PR together !

@OMEGARAZER
Copy link
Contributor Author

You worried me with the before block thinking I duplicated a line by accident.

Your changes make perfect sense though, I will make those changes and update when I get back home in a few hours.

@davidpfarrell
Copy link
Contributor

You worried me with the before block thinking I duplicated a line by accident.

LOL copy-pasta strikes again - I fixed it in after version but forgot to adjust before version -- Edited now for plausible deniability of course :)

Allows for setting "COMMAND_DURATION_MIN_SECONDS" to more than 60 seconds without adding to prompt. Previously always showed with lengths over 60 seconds regardless of setting.
Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

I like it !

@davidpfarrell davidpfarrell merged commit af11a50 into Bash-it:master Feb 22, 2023
niontrix pushed a commit to niontrix/bash-it that referenced this pull request May 23, 2023
Allows for setting "COMMAND_DURATION_MIN_SECONDS" to more than 60 seconds without adding to prompt. Previously always showed with lengths over 60 seconds regardless of setting.
douglasjacobsen added a commit to douglasjacobsen/bash-it that referenced this pull request Jan 19, 2024
* upstream/master: (1190 commits)
  Add support to powerline themes to override foreground color (Bash-it#2231)
  ci: Update GitHub actions v2 => v4 (Bash-it#2224)
  docs: Update Bats libraries links (Bash-it#2225)
  fix: bumps go version to 1.21.0 and changes go get to go install
  [terraform] add alias for terraform workspace
  fix (completion): suppress 1091 in brew (Bash-it#2130)
  Allow for longer command min duration (Bash-it#2198)
  Implement yarn completion (Bash-it#2190)
  Fix lint errors in multiple files (Bash-it#2192)
  bug: Use C style strings when checking for invalid alias characters (Bash-it#2188)
  Remove libra chat reference
  Add more aliases for `git branch`, use long form
  remove function wrapper around kubectl alias registration
  bug: Use en_US when fetching EPOCHREALTIME
  bug:Install shellcheck wget (Bash-it#2173)
  fix(theme): use correct escape sequence to avoid weird text overwriting
  chore: Use grep -E / grep -F instead of egrep / fgrep (Bash-it#2164)
  Fixed broken code blocks in troubleshooting.rst
  Removed Bash Dependency section from README and added it to troubleshooting.rst
  Update variable name to match projects.plugin.bash
  ...
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.

None yet

2 participants