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

Lint/vue completions #1780

Merged
merged 2 commits into from
Jan 26, 2021
Merged

Lint/vue completions #1780

merged 2 commits into from
Jan 26, 2021

Conversation

tbhaxor
Copy link
Contributor

@tbhaxor tbhaxor commented Jan 9, 2021

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

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 Jan 9, 2021

@NoahGorny Lint is failing

If am using string in COMPREPLY the completions are breaking

image

Also I am getting this error on the console after running "./lint_clean_files.sh"

In completion/available/vuejs.completion.bash line 43:
                COMPREPLY=($(compgen -W "-h --help -v --version create add invoke inspect serve build ui init config outdated upgrade migrate info" -- "$curr"))
                           ^-- SC2207: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

Last but not the least the shfmt command is not working properly

image

@NoahGorny
Copy link
Member

I think SC2207 should be addressed, but the shfmt problem is a known one (#652)
I say you should fix the shellcheck error and we will wait for a shfmt cleanup, at the meantime, you can just run shfmt multiple times to get rid of the bug @tbhaxor

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Jan 15, 2021

I think SC2207 should be addressed, but the shfmt problem is a known one (#652)
I say you should fix the shellcheck error and we will wait for a shfmt cleanup, at the meantime, you can just run shfmt multiple times to get rid of the bug @tbhaxor

Do you have fix for it? When i read with IFS=' ' read -a COMPREPLY < <(compgen -W "..." -- "$curr"), completions are breaking

@NoahGorny
Copy link
Member

I think SC2207 should be addressed, but the shfmt problem is a known one (#652)
I say you should fix the shellcheck error and we will wait for a shfmt cleanup, at the meantime, you can just run shfmt multiple times to get rid of the bug @tbhaxor

Do you have fix for it? When i read with IFS=' ' read -a COMPREPLY < <(compgen -W "..." -- "$curr"), completions are breaking

Well @tbhaxor, you are correct. I opened #1791 to ignore this lint error, so just leave the line as it is, and run shfmt a few times

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Jan 18, 2021

@NoahGorny I tried shfmt for multiple times (~10 times) but nothing is changed. Same formatting with spaces

@NoahGorny
Copy link
Member

@NoahGorny I tried shfmt for multiple times (~10 times) but nothing is changed. Same formatting with spaces

Try to update your PR with the latest master, and let me know if there is still a problem like this

@tbhaxor
Copy link
Contributor Author

tbhaxor commented Jan 23, 2021

@NoahGorny I guess its ready to be merged with master

tbhaxor and others added 2 commits January 23, 2021 23:14
Also move the shellcheck warnings to be on their respective line
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

I have made some changes, mostly moved the shell ignore directive to be under each line that violates it, and remove the uneeded condition. @tbhaxor you are welcome to take a look, nice work 😄

@NoahGorny NoahGorny merged commit 4f8fd59 into Bash-it:master Jan 26, 2021
@tbhaxor tbhaxor deleted the lint/vue-completions branch March 11, 2021 11:04
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