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

completion/bash-it: lint and simplify #2029

Merged
merged 5 commits into from
Jan 25, 2022

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Jan 10, 2022

Description

  • shfmt
  • shellcheck
  • Use _bash-it-component-list-enabled() and _bash-it-component-list-disabled() instead of reinventing the wheel.
  • Use read -ra to populate $COMPREPLY, instead of unquoted word separation.
  • New function _compreply_candidates() to simplify the loop for read -ra.

Motivation and Context

After the recent performance upgrades for this completion, I took a look at what was bogging it down before and decided to lint the file. I then fell down the rabbit hole and accidentally refactored it again. I preserved the structure and speed, but deleted quite a lot by using existing functions as well as ensuring we handle spaces and International/unicode characters properly.

How Has This Been Tested?

Manual testing at the command line, test/completion/bash-it.completion.bats, and the full test suite.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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.

@gaelicWizard gaelicWizard force-pushed the completion/bash-it branch 2 times, most recently from 43464e1 to d3095da Compare January 10, 2022 05:31
@gaelicWizard gaelicWizard changed the title DRAFT: completion/bash-it: lint and simplify completion/bash-it: lint and simplify Jan 10, 2022
@gaelicWizard
Copy link
Contributor Author

@tsiflimagas, could you take a look at this one? I accidentally fell down the rabbit hole after you refactored this completion and...refactored it again! Pls forgive me!

This version should, I hope, be shorter code and should support international characters (including spaces and unicode), and should use existing functions from lib/utilities and lib/helpers. And, of course, I've run it through shfmt and shellcheck.

@gaelicWizard gaelicWizard force-pushed the completion/bash-it branch 3 times, most recently from 8cdcca2 to cadbd04 Compare January 10, 2022 07:04
@gaelicWizard gaelicWizard marked this pull request as ready for review January 10, 2022 07:13
@tsiflimagas
Copy link
Contributor

I accidentally fell down the rabbit hole after you refactored this completion and...refactored it again! Pls forgive me!

Haha😆 That's great actually! Your changes are very nice, it's neat how @(mini|simpli)fied the code looks now.
The only thing that got a bit regressed is the enable/disable completions. The work you've done on your other branches, improves that big time, but still, with this caching logic, the first shot is pretty slow (though it becomes very snappy afterwards). That was a pretty clever thought, but I think it's better to always parse the filenames, instead of depending on _bash-it-describe for that (no matter how optimized, it will always be kinda slow, which is fine for printing lists). Thus, I think a bit of rework should be done on the respective functions, though this can go pretty far (totally deprecate the caching logic(?)). That said, I'd be interested to know what your thoughts are on this matter.

@gaelicWizard
Copy link
Contributor Author

@tsiflimagas, I find that the code in this repo is full of rampant layering violations, but it's slow going cleaning things up! My general idea is that the component functions (the ones that touch files, i.e., _bash-it-component-*) ought to depend on no other part of Bash It and ought to be tested within an inch of their life. Anything that runs rm as a matter of course should be subjected to enhanced interrogation for every commit!

I'm definitely not a fan of running _bash-it-describe just to test if a component is enabled, but as you know that's in another PR!

@gaelicWizard
Copy link
Contributor Author

🙃 Test is failing because GitHub's OSX runner is so frakking slow!!! 😭

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.

LGTM

This allows for spaces and special characters in file names, i.e. internaltional/unicode words.
...to `_bash-it()`.

The norm is for the completion function for, e.g., `teh_cmd`. to be named with the same name and a prepended underscore, i.e. `_teh_cmd`. This alsö reduces namespace confusion, which will be relevant in a future patch.
@gaelicWizard
Copy link
Contributor Author

Now that #1934 is merged, I believe that this should no longer stumble through _bash-it-describe() at all. 😃

@NoahGorny NoahGorny merged commit 457a279 into Bash-it:master Jan 25, 2022
@gaelicWizard gaelicWizard deleted the completion/bash-it branch January 25, 2022 16:06
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.

3 participants