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

Update grep.sh #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update grep.sh #2

wants to merge 3 commits into from

Conversation

bdeanindy
Copy link

Modifying to remove the references to the now deprecated GREP_OPTIONS which throws a deprecation warning.

Modifying to remove the references to the now deprecated `GREP_OPTIONS` which throws a deprecation warning.
@zannalov
Copy link
Owner

@bdeanindy

  • Using BASH_ALIASES makes it non-POSIX compliant. Aliases which begin with themselves will not cause infinite recursion, so it would be better to write alias grep="grep ..."
  • Did you mean to use $my_grep_options instead of the literal string my_grep_options in the alias?
  • The purpose of using environment variables so that they would apply in sub-processes, not only when called directly by the shell (for example this approach doesn't work inside Vim). Unfortunately this seems to be the specific use-case they're trying to prevent, and writing a wrapping script which prevents the native grep from being called can get messy, so I don't expect this point to be addressed. Bah...
  • I had the options on separate lines for readability. With the new approach, this could be:
my_grep_options=(
    ...
)
  • I was previously assigning to an environment variable which is persisted/kept. In this instance my_grep_options doesn't need to be kept around after the alias is set up, so at the end of the script you should unset my_grep_options
  • I have a separate file specifically for aliases, so grep.sh should be removed and the new alias should be added to the alias.sh file

Removing per comment on related PR zannalov#2 Update to prevent GREP_OPTIONS deprecation warnings.
@bdeanindy
Copy link
Author

bdeanindy commented Jan 19, 2017

Do you mean like this in alias.sh:

my_grep_options=(--color=auto) # I like color, without deprecation warnings

if ( _version_gte "$( grep -V | head -n 1 | sed 's/[^0-9]*\([0-9]*\.[0-9]*\).*/\1/' )" "2.5.2" ); then
    # The following adds new options for grep to exclude: CVS, .cvs, .git, .hg, .svn directories
    my_grep_options=(
        ${my_grep_options[@]}
            --exclude-dir=.cvs
            --exclude-dir=.git
            --exclude-dir=.hg
            --exclude-dir=.svn
            --exclude-dir=CVS
    )
    alias grep="grep $my_grep_options"
    unset my_grep_options
fi

Or did you mean like this...

my_grep_options=(--color=auto) # I like color, without deprecation warnings

if ( _version_gte "$( grep -V | head -n 1 | sed 's/[^0-9]*\([0-9]*\.[0-9]*\).*/\1/' )" "2.5.2" ); then
    # The following adds new options for grep to exclude: CVS, .cvs, .git, .hg, .svn directories
    my_grep_options=(
        ${my_grep_options[@]} --exclude-dir=CVS)
        ${my_grep_options[@]} --exclude-dir=.cvs)
        ${my_grep_options[@]} --exclude-dir=.git)
        ${my_grep_options[@]} --exclude-dir=.hg)
        ${my_grep_options[@]} --exclude-dir=.svn)
    )
    alias grep="grep $my_grep_options"
    unset my_grep_options
fi

Per the PR zannalov#2 feedback from @zannalov:

Improve source for readability.
Fix invalid variable reference in alias command string.
Unset at end of execution.
@bdeanindy
Copy link
Author

@zannalov This is operating as expected on my local installation and I believe it addresses all the issues you surfaced in the PR comments.

Let me know if you need any other changes on this sir.

${my_grep_options[@]} --exclude-dir=.cvs) # Excludes .cvs directories for CVS
${my_grep_options[@]} --exclude-dir=.git) # Excludes .git directories for Git
${my_grep_options[@]} --exclude-dir=.hg) # Excludes .hg directories for Mercurial
${my_grep_options[@]} --exclude-dir=.svn) # Excludes .svn directors for Subversion
Copy link
Owner

Choose a reason for hiding this comment

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

Syntax error, too many closing parenthesis (one per line)

Additionally (if it weren't for the syntax error) this would result in --color=auto --exclude-dir=CVS --color=auto --exclude-dir=.cvs --color=auto --exclude-dir=.git --color=auto --exclude-dir=.hg --color=auto --exclude-dir=.svn -- You only need ${my_grep_options[@]} once up-front.

${my_grep_options[@]} --exclude-dir=.svn) # Excludes .svn directors for Subversion
)
alias grep="grep $my_grep_options"
unset my_grep_options
Copy link
Owner

Choose a reason for hiding this comment

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

These two lines should be outside the if-block so that --color=auto is applied even if --exclude-dir isn't supported.

${my_grep_options[@]} --exclude-dir=.hg) # Excludes .hg directories for Mercurial
${my_grep_options[@]} --exclude-dir=.svn) # Excludes .svn directors for Subversion
)
alias grep="grep $my_grep_options"
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest using the same ${my_grep_options[@]} here

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I follow since this conflicts with your initial feedback sir:

so it would be better to write alias grep="grep ..."

Copy link
Author

Choose a reason for hiding this comment

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

Oh I think I see what you mean. Instead of the $my_grep_options use ${my_grep_options[@]}...right?

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