pelias / docker

Run the Pelias geocoder in docker containers, including example projects.
MIT License
314 stars 217 forks source link

fix: grep pattern fixed to remove line comments #311

Open langbein-daniel opened 1 year ago

langbein-daniel commented 1 year ago

:wave: 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 :rocket:

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

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

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.

missinglink commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 :+1:

langbein-daniel commented 1 year ago

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:

Inside env_load_stream() we then split the lines by the first occurrance of =. So ` andabresult in errors there. But a=bwould result inexport " a=b". This causesexportto fail withexport: a=b': not a valid identifier.

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