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

fix: grep pattern fixed to remove line comments #311

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

Conversation

langbein-daniel
Copy link

👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.


Here's the reason for this change 🚀

  1. Running pelias yields the error grep: warning: stray \ before #.
  2. Line comments were not removed by grep pattern.

Here's what actually got changed 👏

  1. This can be fixed by not escaping # in the grep pattern.
  2. The grep pattern: There was a end-of-line anchor $ after a pattern for white-spaces \s* before the line-comment character #. This was changed to ^\s*# which remove lines starting with 0 or more whitespace characters followed by #.

Here's how others can test the changes 👀

Run the pelias command and check if loading .env variables does still work as expected/is now fixed. Especially if line comments (with and without whitespace before #) get removed.

Running `pelias` yields the error `grep: warning: stray \ before #`. This can be fixed by not escaping `#` in the grep pattern.
The second part of the grep pattern does now remove lines starting with 0 or more whitespace characters followed by #.
@missinglink
Copy link
Member

missinglink commented Feb 28, 2023

Thanks for the bugfix.

I don't see this warning on my terminal, could you please post the following:

uname -a
Darwin Peters-MacBook-Pro-2.local 21.3.0 Darwin Kernel Version 21.3.0: Wed Jan  5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_X86_64 x86_64 i386 Darwin

grep --version
grep (BSD grep, GNU compatible) 2.6.0-FreeBSD

I wonder if we can use this pattern which seems much easier to read:

grep -Ev '^$|^#' .env

or maybe this to maintain backwards compatibility:

grep -v '^\s*[$#]' .env

Also wondering why the pipe still needs to be escaped \| 🤷

@langbein-daniel
Copy link
Author

I don't have a link to the doc handy right now, but the | needs to be escaped in order to combine both patterns with a logical or. This can easily be tested with this, which should result in only one line bar and not in a blank one or in #:

echo '
#
bar' | grep -v '^$\|^\s*#'

The pattern grep -v '^\s*[$#]' .env does unfortunately not remove blank lines.

I'm using Arch Linux, the exact versions are:

$ uname -a
Linux yodaTux 6.2.1-arch1-1 #1 SMP PREEMPT_DYNAMIC Sun, 26 Feb 2023 03:39:23 +0000 x86_64 GNU/Linux

$ grep --version
grep (GNU grep) 3.8
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Mike Haertel and others; see
<https://git.sv.gnu.org/cgit/grep.git/tree/AUTHORS>.

@langbein-daniel
Copy link
Author

As we have the comment note: strips comments and empty lines just before the pattern, I don't think that it's too problematic if the grep pattern is not super easy to read. So I would rather keep backward compatibility.

Apart from that, grep -Ev '^$|^#' .env seems to work 👍

@langbein-daniel
Copy link
Author

One last thing: We are currently removing blank lines as well as lines having # as first non-whitespace character. This leaves lines starting with whitespace characters not directly followed by # untouched. Some examples are:

  • a=b
  • ab

Inside env_load_stream() we then split the lines by the first occurrance of =. So and ab result in errors there. But a=b would result in export " a=b". This causes export to fail with export: a=b': not a valid identifier`.

So, all cases are covered. But I think it would be good to document this a bit better.

One subshell `(...) &&` was replaced with `if [...]; then`.
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