lint: add eslint config, fix lint issues, enforce clean lint status pre-commit and in CI#64
Closed
mikehardy wants to merge 8 commits intostyfle:mainfrom
mikehardy:lint
Closed
lint: add eslint config, fix lint issues, enforce clean lint status pre-commit and in CI#64mikehardy wants to merge 8 commits intostyfle:mainfrom mikehardy:lint
mikehardy wants to merge 8 commits intostyfle:mainfrom
mikehardy:lint
Conversation
The prettier config was adapted from the official GitHub Actions repo, bent to fit the prevailing style (where possible) already in the project The intent is not to be controversial or argue about whitespace, it is just to have a consistent easy-to-verify style specifically to avoid all arguments about whitespace. If anything in here is objectionable, just name the setting to alter and I can edit / re-format / re-push
This enforces the same checks locally that will execute in CI With this, everyone should have a clean / consistent dev environment, and it will be clear to contributors if they submit code that is not valid typescript Additionally, after doing the build it adds the dist/index.js output to the commit list so contributors can't forget to commit it
Similar to the prettier config, this is adapted from the main GitHub Actions repo, and the intent is not to be controversial it is simply to have any consistent / easy to check standard. If anything is objectionable, just point out the setting The config was adapted from the main repo to match what appeared to be prevailing opinion on this project (semicolons preferred, bracket spacing preferred, etc) The following commits will fix the small errors that seemed worth fixing
Owner
|
I think prettier is sufficient (PR #63), no need for eslint |
Contributor
Author
|
Professionally I disagree, it pointed out at least one problem that had semantic interest, that of a shadowed variable, and in future maintainer conversations may devolve into style vs content (the main thing eslint forestalls) but of course as maintainer you can disagree :-). Happy to see forward progress in the repo either way Closing |
Owner
|
I appreciate the PRs! In the future, eslint could be a good addition, just not at this time 🙂 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Similar to #63 where an auto-formatter was added, this adds an automated lint check
It is based on #63 and will be re-based to remove those commits if/when 63 has progress but I wanted to post this now for discussion.
There are no functional changes in this PR, it is all minor code tweaks that do not have semantic differences
Also similar to #63 I want to be clear that I have opinions on code lint just like everyone, and they don't matter. What matters is that the project has a consistent one and it's enforced so contributors know what's okay and what isn't, so it follows that getting it merged is more important than any specific rule.
Given that, I took the eslint config from the main github actions repo, then altered the rules to fit what I thought were the preferences here. If any given rule is wrong, I'll alter the config and re-push until it's good
That said it appeared there were 4 real issues which I fixed in single commits
Once everything passed, I added the lint check to the CI lint workflow and the husky pre-commit hook.
I won't post other PRs using commits from #62 until these are resolved, since the review is otherwise impossible with all the formatting change