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.

Sign up for GitHub

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

Jump to bottom

Add editorconfig-checker #221

Merged
merged 4 commits into from
Jul 6, 2020
Merged

Add editorconfig-checker #221

merged 4 commits into from
Jul 6, 2020

Conversation

mstruebing
Copy link
Contributor

@mstruebing mstruebing commented Jun 21, 2020 •
edited

see: https://github.com/editorconfig-checker/editorconfig-checker/

I hope this kind of linter is allowed as it is not for linting a specific language.

@mstruebing mstruebing force-pushed the addEditorconfigChecker branch 4 times, most recently from ddaf464 to 83873b5 Compare June 23, 2020 08:49
@mstruebing
Copy link
Contributor Author

Any news on this?

@admiralAwkbar
Copy link
Collaborator

@mstruebing this is interesting...
With all the various languages being listed, wouldn't this cause issues?
I.e. I have python and bash in a repo, one uses tabs, one uses spaces.

@admiralAwkbar admiralAwkbar added enhancement New feature or request help wanted Extra attention is needed labels Jun 24, 2020
@mstruebing
Copy link
Contributor Author

mstruebing commented Jun 24, 2020 •
edited

As long as the .editorconfig have the correct rules this isn't a problem at all.
With an .editorconfig you are able to define basic formatting for different filenames.
See: https://editorconfig.org/

@ahmadnassri ahmadnassri mentioned this pull request Jul 1, 2020
Closed
nemchik
nemchik reviewed Jul 1, 2020
View reviewed changes
lib/linter.sh
# Lint the files with editorconfig #
####################################
# LintCodebase "FILE_TYPE" "LINTER_NAME" "LINTER_CMD" "FILE_TYPES_REGEX" "FILE_ARRAY"
LintCodebase "EDITORCONFIG" "editorconfig-checker" "editorconfig-checker" "^.*$" "${FILE_ARRAY_ENV[@]}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this going to lint all dotfiles?

Is this linter supposed to lint all dotfiles? Or just the .editorconfig files?

Ex: should this lint .gitignore ? It looks like it will.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that a regex which says: "start with any character n times and then end"?
So it will lint all files, by that it will consider your .editorconfig file if present.
So to be clear: the .editorconfig is the file which describes how your files should be formatted (basically, tabs/spaces, spaces amount, line ending character, like that), which can be used for any filetype and programming language. It's language agnostic. You can have wildcard rules or make rules for specific files or file globs.
So it will lint every file.

Does this explain it properly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I misread the regex. Sorry. You are correct that pattern will match any file in the repo.

So my question then changes: Is this linter meant to lint .editorconfig files, or is it meant to check that all files comply with the settings specified in the .editorconfig that applies to them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's exactly the second one. It checks that your files comply with your settings specified in .editorconfig.
Sorry for not explaining it properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes more sense. What happens in a repo with no .editorconfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No error should occur, if there is no definition for a specific check it is skipped and not assuming something defaultish.

.automation/test/editorconfig-checker/editorconfig-checker_bad_1.ext
@@ -0,0 +1,3 @@
some line
Copy link
Collaborator

Choose a reason for hiding this comment

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

Depending on the answers to my other questions these files might need to be split into subfolders named good and bad, and the files might need to be named .editorconfig in each folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense now with the other conversation we've had.

nemchik
nemchik requested changes Jul 2, 2020
View reviewed changes
Copy link
Collaborator

@nemchik nemchik left a comment

Choose a reason for hiding this comment

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

I like the ideas here. This branch has fallen a little behind (development in this repo is turned up to 11 right now). Can you rebase on the current master and resolve the conflicts?

@mstruebing
Copy link
Contributor Author

Currently rebased :)

@mstruebing mstruebing force-pushed the addEditorconfigChecker branch 2 times, most recently from f9e8792 to 0075e70 Compare July 3, 2020 07:19
@zkoppert
Copy link
Contributor

zkoppert commented Jul 6, 2020

resolved conflicts caused by the html PR getting merged in first.

@zkoppert
Copy link
Contributor

zkoppert commented Jul 6, 2020

@nemchik Do you believe this one is ready?

@nemchik
Copy link
Collaborator

nemchik commented Jul 6, 2020

LGTM.

@zkoppert
Copy link
Contributor

zkoppert commented Jul 6, 2020

fixing up my merge mistakes and then sending this through if it passes CI.

@zkoppert zkoppert self-assigned this Jul 6, 2020
@zkoppert zkoppert merged commit 159a1b5 into super-linter:master Jul 6, 2020
@GaboFDC GaboFDC mentioned this pull request Jul 7, 2020
Merged
2 tasks
@GaboFDC GaboFDC mentioned this pull request Sep 24, 2020
Merged
3 tasks
@snyk-bot snyk-bot mentioned this pull request Oct 30, 2021
Closed
@snyk-bot snyk-bot mentioned this pull request Nov 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nemchik nemchik nemchik requested changes

@zkoppert zkoppert zkoppert approved these changes

@admiralAwkbar admiralAwkbar Awaiting requested review from admiralAwkbar

@jwiebalk jwiebalk Awaiting requested review from jwiebalk

@IAmHughes IAmHughes Awaiting requested review from IAmHughes

Assignees

@zkoppert zkoppert

Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mstruebing @admiralAwkbar @zkoppert @nemchik