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.

Sign up for GitHub

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

Jump to bottom

chore: Use grep -E / grep -F instead of egrep / fgrep #2164

Merged
merged 1 commit into from
Oct 13, 2022
Merged

chore: Use grep -E / grep -F instead of egrep / fgrep #2164

merged 1 commit into from
Oct 13, 2022

Conversation

davidpfarrell
Copy link
Contributor

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

  • 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.

@davidpfarrell davidpfarrell mentioned this pull request Sep 12, 2022
Closed
@davidpfarrell
Copy link
Contributor Author

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

NoahGorny
NoahGorny approved these changes Sep 29, 2022
View reviewed changes
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 took a look and I think it looks great, sadly I cannot test all of the changes, but they are well explained. Well done @davidpfarrell

I think we should merge this, and I will release a new stable tag :)

@cornfeedhobo
Copy link
Member

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

This was referenced Oct 2, 2022
Closed
Closed
@davidpfarrell davidpfarrell changed the title chore: Review usages of [ef]grep, shift to grep -[EF] chore: Use grep -E / grep -F instead of egrep / fgrep Oct 7, 2022
@davidpfarrell
Copy link
Contributor Author

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
Copy link
Contributor Author

davidpfarrell commented Oct 13, 2022 •
edited
Loading

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 davidpfarrell merged commit 00062bf into Bash-it:master Oct 13, 2022
@davidpfarrell davidpfarrell deleted the chore/egrep branch October 13, 2022 17:35
@NoahGorny
Copy link
Member

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 :)

Royalsspirit pushed a commit to Royalsspirit/bash-it that referenced this pull request Oct 14, 2022
@davidpfarrell
chore: Use grep -E / grep -F instead of egrep / fgrep (Bash-it#2164)
9db814c 
Ensures that the -E or -F option, when used, is the first option
* i.e. 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
@cornfeedhobo
Copy link
Member

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

douglasjacobsen added a commit to douglasjacobsen/bash-it that referenced this pull request Jan 19, 2024
@douglasjacobsen
Merge remote-tracking branch 'upstream/master'
b554ce9 
* 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
Reviewers

@NoahGorny NoahGorny NoahGorny approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: "egrep: warning: egrep is obsolescent"
3 participants
@davidpfarrell @cornfeedhobo @NoahGorny