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

Implement yarn completion #2190

Merged
merged 5 commits into from
Feb 22, 2023
Merged

Conversation

tbhaxor
Copy link
Contributor

@tbhaxor tbhaxor commented Feb 1, 2023

Description

This PR will implement yarn completion from the external vendor

Motivation and Context

Yarn is required while developing the nodejs projects and often as project grows completion is required. This PR fixes that problem.

How Has This Been Tested?

Locally by sourcing

Screenshots (if appropriate):

image

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.

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Feb 9, 2023

@NoahGorny @davidpfarrell could you review it and merge. I need it urgently

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.

Hey ! So the .bash file doesn't match the pattern used in other vendor competions:
django.completion.bash

# shellcheck shell=bash
about-completion "django completion"
# shellcheck disable=SC1090
source "${BASH_IT}"/vendor/github.com/django/django/extras/django_bash_completion

apm.completion.bash

# shellcheck shell=bash
about-completion "apm completion"
# shellcheck disable=SC1090
source "${BASH_IT}"/vendor/github.com/vigo/apm-bash-completion/apm

jboss5.completion.bash

# shellcheck shell=bash
about-completion "jboss5 completion"
# shellcheck disable=SC1090
source "${BASH_IT}"/vendor/github.com/rparree/jboss-bash-completion/jboss5

jboss7.completion.bash

# shellcheck shell=bash
about-completion "jboss7 completion"
# shellcheck disable=SC1090
source "${BASH_IT}"/vendor/github.com/rparree/jboss-bash-completion/jboss7

Mind taking a quick pass to normalize yours?
Also wondering why 2207 vs 1090 in your case?

-D

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Feb 10, 2023

@davidpfarrell Any reason why arent we using git submodules?

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Feb 10, 2023

Also wondering why 2207 vs 1090 in your case?

Fixed lint errors

@davidpfarrell
Copy link
Contributor

@davidpfarrell Any reason why arent we using git submodules?

A couple of reasons.

We sometimes need to modify the file in order to make it work within the bash-it framework ie. calling some of our functions, etc ...

We don't necessarily need to bring down the entire repo.

Bash-it doesn't currently utilize submodules outside of the testing, so regular users don't have to deal with it right now, so bringing it into the standard user experience is a big decision (imho)

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'm surprised the lint check didn't hit the SC1090 - Although I realize now that the other completions are dealing with it wrong ie. we should use source= instead of disabling it.

So checking the repository for the completion, I notice two things:

  • This is already available in homebrew bash-completions
  • This only officially supports Bash ^4.x

Not sure if any doc changes or checks should be added but wanted to call it out ...

Changes I do recommend:

  • Add repo url to about - Not sure if there's a built-in meta tag for it?
  • Add the SC1090 line from the examples - Not sure why its not hitting but I'm sure it will in the future and don't want to break future builds because of it.

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Feb 14, 2023

@davidpfarrell @NoahGorny can we merge this if everything is fine. My team is waiting for this

@davidpfarrell
Copy link
Contributor

@davidpfarrell @NoahGorny can we merge this if everything is fine. My team is waiting for this

Sorry @tbhaxor my previous comment was a request-for-changes comment, re:

Changes I do recommend:

Add repo url to about - Not sure if there's a built-in meta tag for it?
Add the SC1090 line from the examples - Not sure why its not hitting but I'm sure it will in the future and don't want to break future builds because of it.

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Feb 15, 2023

@davidpfarrell SC1090 is not triggering on my end either, but 1091 is triggering. Do you want me to disable it also?

Screenshot_20230215_071152

@davidpfarrell
Copy link
Contributor

@tbhaxor Does 1091 trigger even when disable=SC1090 is present?

The two codes are related and in this case 1091 is being triggered by new-ish behavior for 1090 errors.

But it's unclear from the docs if disabling 1090 also disables 1091 in this special case.

My thought is is might so try just disabling 1090 and see if that clears ...

Alternatively you could add source=../../vendor/github.com/dsifford/yarn-completion/yarn and make shellcheck happy :)

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Feb 22, 2023

Again, where did you get it triggered or just assuming it could trigger?

@davidpfarrell
Copy link
Contributor

Failing lint errors presumed fixed in #2192 which has been merged ...

@davidpfarrell davidpfarrell merged commit 05ef68a into Bash-it:master Feb 22, 2023
@tbhaxor tbhaxor deleted the feat/yarn-completion branch February 22, 2023 17:59
niontrix pushed a commit to niontrix/bash-it that referenced this pull request May 23, 2023
* feat (completion): add yarn completion
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.

2 participants