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

Setup linters #102

Merged
merged 11 commits into from
Dec 13, 2017
Merged

Setup linters #102

merged 11 commits into from
Dec 13, 2017

Conversation

loicteixeira
Copy link
Contributor

@loicteixeira loicteixeira commented Dec 1, 2017

Add Linters dependencies & config for Python and JS. Waiting on prettier/prettier#3038 for SASS.

I'll add the steps to the CI after merging.

Makefile Outdated
find . -name '*~' -exec rm -f {} +

lint-py: ## Run Python linters
flake8 $(files)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately files passed explicitly in the CLI have precedence over the exclude list of the config.
For example, if you create a Django migration, it will be parsed by flake8 (and fail because of line length) despite being ignore.

I don't think we can use flake8 default githook but maybe the hook could use Python instead and take advantage of update_excludes.


for HOOK in $HOOKS
do
source $HOOK "$CHANGED_FILES"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to allow other scripts than bash scripts (e.g. python script), we might need to replace this with $HOOK "$CHANGED_FILES" and add execution permission to the pre-commit-* files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be reasonable to assume all files in hooks are executable. Is this the only reason why you used source, or does it do something else?

@holloway
Copy link

holloway commented Dec 1, 2017

@loicteixeira It's unclear whether Prettier will adopt the Indenting CSS PR.

They ran a Twitter poll for 30 hours among likely Prettier users who, if they used Prettier, couldn't use the indenting style, and the results were that 9% of about 800 people use the indenting style despite Prettier lacking the feature (presumably they disable Prettier CSS/Sass formatting).

They also seemed to reject far larger polls of about 3400 users where 30% of devs said they prefer an indenting style, based on unsubstantiated suspicions by one dev of theirs.

So it's a very odd situation, and the PR is in limbo right now. It's unclear what threshold needs to be met to accept it, and although the code is complete I wouldn't wait on it.

Sad!

Copy link
Contributor

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

I'm late to the party, just added a couple questions 🙂 .

Also I'm surprised you didn't have to add overrides for the Makefile in the .editorconfig, like https://github.com/springload/draftjs_exporter/blob/master/.editorconfig:

# Makefiles are special.
[Makefile]
indent_style = tab
indent_size = 8


for HOOK in $HOOKS
do
source $HOOK "$CHANGED_FILES"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be reasonable to assume all files in hooks are executable. Is this the only reason why you used source, or does it do something else?

@@ -0,0 +1,5 @@
JS_FILES=$(echo "$CHANGED_FILES" | { grep "\.js$" || true; })

[[ -z "$JS_FILES" ]] && return
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's so much cleaner than all the ifs!

@loicteixeira
Copy link
Contributor Author

loicteixeira commented Dec 5, 2017

As discussed offline, we will look at either using Prettier with the default indenting style (without the PR mentioned above merged) or something else as it is unclear when/if the PR will be merged unfortunately.

Loic Teixeira added 2 commits December 12, 2017 17:52
This allows scripts to use whichever language instead of enforcing bash.
@loicteixeira
Copy link
Contributor Author

loicteixeira commented Dec 12, 2017

@thibaudcolas:

  • It seems that VisualStudio Code does not respect .editorconfig for Makefiles and use tabs of size 4 anyway (which is why it worked so far) but I've added the config as not everybody might use VSCode.
  • I've made all the hooks executable and removed the source but FWIW, we can't use return anymore but have to use exit 0 instead as you can only return from within a function or a sourced script.

Edit: I can't quite get the CSS linting to work 😞

@loicteixeira
Copy link
Contributor Author

Merging without CSS linting as I can't quite get it to work but need to get on something else. I'll create a separate issue to revisit.

@loicteixeira loicteixeira merged commit cdb4f08 into master Dec 13, 2017
@loicteixeira loicteixeira deleted the chore/linters branch December 13, 2017 20:18
@loicteixeira loicteixeira mentioned this pull request Dec 13, 2017
4 tasks
@thibaudcolas
Copy link
Contributor

🎉

For me VS Code respects the settings defined for https://github.com/springload/draftjs_exporter, I might have a plugin doing that though not sure.

b-jam pushed a commit that referenced this pull request Nov 29, 2019
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

3 participants