bash-it icon indicating copy to clipboard operation
bash-it copied to clipboard

chore: Review usages of [ef]grep, shift to grep -[EF]

Open davidpfarrell opened this issue 3 years ago • 1 comments

Takes a pass at correcting usages of egrep and fgrep, replacing with grep -E and grep -F, respectively.

Description

Also makes a few other modifications:

  • Ensures that the -E or -F option, when used, is the first option
    • ie. grep -oE => grep -E -o
  • Updates _bash-it-grep to invoke grep with just the provided arguments
    • This function was (and still is) unused, but decided this new functionality was actually more useful
  • Introduces _bash-it-fgrep to invoke grep -F
  • Removes type -P egrep from the _bash-it-*grep functions
  • For usages that were already going to be modified, use -F if appropriate
    • Does not touch grep usages that may have benefited from -F, but were not otherwise considered for this PR
  • Adds shellcheck header to modified .bash files that didn't already have it

Motivation and Context

The egrep and fgrep tools were never posix standards. As of the newly released v3.8 GNU grep now loudly reports a warning when invoking these tools as egrep and fgrep.

Fixes #2163

How Has This Been Tested?

This is NOT been tested.

In fact, I had to disable the shellcheck and shfmt hooks in order to get this to commit.

I took a quick run at trying to correct the shellcheck and shfmt errors, but there are JUST SO MANY of them, that the actual egrep changes would get lost in the noise.

Types of changes

  • [x] 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:

  • [x] My code follows the code style of this project.
  • [ ] If my change requires a change to the documentation, I have updated the documentation accordingly.
  • [x] 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.

davidpfarrell avatar Sep 12 '22 00:09 davidpfarrell

I left a bunch of comments throughout the changes files to hopefully explain what I was thinking ...

davidpfarrell avatar Sep 12 '22 00:09 davidpfarrell

I can test this over the coming week on linux. I don't have a mac available anymore.

cornfeedhobo avatar Oct 02 '22 16:10 cornfeedhobo

Hi Team!

@cornfeedhobo any findings from testing this?

I think its time to merge - We've had 2 other PRs open on the matter so the natives are getting restless :)

I usually don't like to do merges and leave that to other team members, but I may merge this one later today or tomorrow morning ...

davidpfarrell avatar Oct 07 '22 15:10 davidpfarrell

Not sure why mac jobs are failing or why re-running them (eventually) allows them to pass ... But we know the lint job is failing due to brew missing, sooo ...

Ship it !

davidpfarrell avatar Oct 13 '22 17:10 davidpfarrell

We ought to fix the lint stage, and probably understand what's going on with the Mac tests

Anyway, thanks for creating and merging the pr @davidpfarrell :)

NoahGorny avatar Oct 13 '22 17:10 NoahGorny

@davidpfarrell sorry I didn't reply in time, but yeah, this wasn't causing issues for me. thanks for moving on it!!

cornfeedhobo avatar Oct 19 '22 01:10 cornfeedhobo