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

Editorconfig special case #391

Merged
merged 3 commits into from
Jul 8, 2020
Merged

Editorconfig special case #391

merged 3 commits into from
Jul 8, 2020

Conversation

GaboFDC
Copy link
Collaborator

@GaboFDC GaboFDC commented Jul 7, 2020

Following the logic from here if we don't specify any linter flag (i.e use all linters) then, editorconfig will run against the entire code base, even if there is no .editorconfig present, and it will even check the .git/ folder.

Proposed Changes

In some sense this is expected because we are not specifying any linter, so all should be run. But as this one is special and lints the entire code base, I think is worth putting an extra check to only do it if either the flag is set, or .editorconfig is present as a special case because this behavior is different from other linters (run only against files from that extension/type).

I'm open to all feedback here. specially @mstruebing

PD: I think another needed change (independent of this one) is to exclude .git/ from editor config.

PD2: Not sure if this is a typo or what is the purpose of this?

Readiness Checklist

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@nemchik
Copy link
Collaborator

nemchik commented Jul 7, 2020

@nemchik
Copy link
Collaborator

nemchik commented Jul 7, 2020

I am guessing this branch was based on the branch used in https://github.com/github/super-linter/pull/372
could you rebase on master from this repo so we can see the changes separately?

@GaboFDC
Copy link
Collaborator Author

GaboFDC commented Jul 7, 2020

I am guessing this branch was based on the branch used in #372
could you rebase on master from this repo so we can see the changes separately?

Yes sorry, my mistake, it is fixed now

@mstruebing
Copy link
Contributor

I think it should be done to not run the editorconfig-checker when no .editorconfig is present, even if it could be that you could have .editorconfig-files in subdirectories, but tbh I never saw such a project without also a root .editorconfig-file.

To hide it behind a flag: I think it is okay because it is somehow a bit different behaviour than other linters of this project.

@nemchik
Copy link
Collaborator

nemchik commented Jul 7, 2020

PD: I think another needed change (independent of this one) is to exclude .git/ from editor config.

Agreed.

PD2: Not sure if this is a typo or what is the purpose of this?

Seems like a typo.

#############################
# Editorconfig special case #
#############################
LINTER_RULES_PATH="${LINTER_RULES_PATH:-.github/linters}" # Linter Path Directory
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I am understanding this correctly, if users don't set LINTER_RULES_PATH manually it'll be set to .github/linters and if there is an .editorconfig there then the linter will still run?

Logically I think it makes more sense to not include this block of changes and instead change the logic here https://github.com/github/super-linter/pull/391/files#diff-c7b422f2f140a91b67618e5aa86d8d88R495 to if [ -f "$GITHUB_WORKSPACE/.editorconfig" ];

@mstruebing
Copy link
Contributor

PD2: Not sure if this is a typo or what is the purpose of this?

This was for debugging purpose while developing, I think I missed to remove it and it doesn't get caught while reviewing.

@GaboFDC
Copy link
Collaborator Author

GaboFDC commented Jul 7, 2020

Filed #395 for the .git/ folder

@GaboFDC GaboFDC marked this pull request as ready for review July 7, 2020 14:24
@nemchik
Copy link
Collaborator

nemchik commented Jul 7, 2020

LGTM now.

@admiralAwkbar
Copy link
Collaborator

@GaboFDC @nemchik @mstruebing amazing work in here all!
Really thank you so much for all the hard work

@admiralAwkbar admiralAwkbar merged commit 91ea286 into super-linter:master Jul 8, 2020
@GaboFDC GaboFDC deleted the gf_fix_editorconfig branch July 30, 2020 14:15
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.

None yet

4 participants