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

Add support for .lintignore. #67

Merged
merged 1 commit into from
Jan 25, 2017
Merged

Add support for .lintignore. #67

merged 1 commit into from
Jan 25, 2017

Conversation

marccarre
Copy link
Contributor

Lint-ing all of Weave Net is currently not desirable because:

  • it would create conflicts on many branches,
  • it would pollute the git history,
  • etc.

and because lint currently lints all files except ./vendor, this breaks Weave Net's build.
As a result, Weave Net is not using the latest commit in build-tools's master branch, which is also source of problems:

This change adds the ability to optionally process a .lintignore file, which can contain any number of GLOBs then used to filter out files one does not want to lint, a-la .gitignore.
This, coupled with a .lintignore file in Weave Net will then enable Weave Net to use the latest version of build-tools.

@marccarre marccarre force-pushed the shfmt-ignore branch 2 times, most recently from 315223a to 504eccb Compare January 24, 2017 17:24
@marccarre marccarre self-assigned this Jan 24, 2017
@marccarre marccarre force-pushed the shfmt-ignore branch 5 times, most recently from 59fe2fb to 9adc07e Compare January 24, 2017 19:30
Copy link
Contributor

@ekimekim ekimekim left a comment

Choose a reason for hiding this comment

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

I'd like to see a different approach than transforming the globs into regex. I've outlined one option in the specific comment. I would also accept simply making the .lintignore file actually regex in the first place.
Otherwise LGTM

@@ -171,6 +171,27 @@ lint_files() {
exit $lint_result
}

filter_out() {
local globs="$1"
if [ -n "$globs" -a -r "$globs" ]; then

This comment was marked as abuse.

This comment was marked as abuse.

while read -r filename; do
# Remove comments. Remove empty lines. Expand directories (e.g. "bin/" -> "bin/.*"). And then regexify globs.
# These regexes are then passed to grep to filter out matching elements from stdin:
echo "${filename}" | tee -a "$files_in" | grep -vEf <(sed '/^#.*$/d ; /^\s*$/d ; s/^\(.*\/\)$/\1*/g ; s/\([.|]\)/\\\1/g ; s/\?/./g ; s/\*/.*/g ; s/\(.*\)/\^\1\$/g' "$globs") | tee -a "$files_out"

This comment was marked as abuse.

This comment was marked as abuse.

rm -f "$files_in" "$files_out"
else
# No globs blacklist was provided: simply propagate stdin to stdout:
while read -r filename; do echo "${filename}"; done

This comment was marked as abuse.

This comment was marked as abuse.

@marccarre marccarre force-pushed the shfmt-ignore branch 4 times, most recently from 8b15aee to d9f6532 Compare January 25, 2017 15:31
@marccarre marccarre assigned ekimekim and unassigned marccarre Jan 25, 2017
@marccarre
Copy link
Contributor Author

@ekimekim: I've made a few changes following your first review:

  • removed these awful sed patterns to convert globs to regexes,
  • no longer using temporary files to debug,
  • made .lintignore dependency-injectable (so the name can be overriden, if we don't like this name for whatever reason).

It should be simpler overall, or at least more readable, but I wouldn't mind a second pass from your end.

Copy link
Contributor

@ekimekim ekimekim left a comment

Choose a reason for hiding this comment

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

Mostly nits (and it looks like a mid-review change might have eaten some?) but one important thing to fix.

filter_out() {
local patterns="$1"
if [ -n "$patterns" ] && [ -r "$patterns" ]; then
local patterns=$(sed '/^#.*$/d ; /^\s*$/d' $patterns) # Remove blank lines and comments before we start iterating.

This comment was marked as abuse.

[ -n "$DEBUG" ] && echo >&2 "> Filters:" && echo >&2 "$patterns"
local filtered_out=()
while read -r filename; do
matches_any "$filename" "$patterns" && filtered_out+=("$filename") || echo "$filename"

This comment was marked as abuse.

local patterns="$2"
IFS=$'\n' # Make newlines the only separator.
set -f # Disable globbing so $patterns can be treated as is.
for pattern in $patterns; do

This comment was marked as abuse.

This comment was marked as abuse.

@marccarre
Copy link
Contributor Author

Yes, just force-pushed # shellcheck disable=SC2053 and some renamings.
Changing a few more things based on your comments. Cheers!

Lint-ing all of Weave Net is currently not desirable because:

* it would create conflicts on many branches,
* it would pollute the git history,
* etc.

and because `lint` currently lints all files except `./vendor`, this breaks Weave Net's build.
As a result, Weave Net is not using the latest commit in `build-tools`'s `master` branch, which is also source of problems:

* it limits standardisation of projects across Weaveworks,
* it limits the ability to use the latest features of `build-tools`,
* it complexifies merging changes in `build-tools` (e.g. #53 and weaveworks/weave#2694 for weaveworks/weave#2647),
* etc.

This change adds the ability to optionally process a `.lintignore` file, which can contain any number of `GLOB`s then used to filter out files one does not want to lint, a-la `.gitignore`.
This, coupled with a `.lintignore` file in Weave Net will then enable Weave Net to use the latest version of `build-tools`.
@marccarre marccarre merged commit 10a36b3 into master Jan 25, 2017
@marccarre marccarre deleted the shfmt-ignore branch January 25, 2017 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants