Issue 1379 linting and formatting #1414
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@LlGC-jop, I personally don't mind having a GitHub action that forces correct linting in pull requests -- this ensures that style problems never creep into the main project, since they always get fixed automatically at the pull request level. It's a bit awkward when you introduce new settings, but if you want to avoid that problem, I'd suggest a simple workaround: 1.) Apply the style changes FIRST, and open a PR to update the styles (with a description explaining why the style is being changed). 2.) After the style changes are merged, open a second PR to apply the settings that enforce the styles. Since the style fixes are already merged, this PR will only contain configuration. And now going forward, the only code that should get updated automatically is new code; there should never be weird unrelated style changes. One caveat: if you don't pin tool versions in package.json, there's always the possibility that some minor dependency update (e.g. a bug fix in a linter) will suddenly cause a bunch of files to change. But I don't mind this either; if dependabot opens a PR to update a linting tool, and a bunch of files get their linting changed as part of that PR, it shows the relationship between the changes, and that's useful for history. Of course, I'm open to different perspectives if others disagree! |
|
Also, a suggestion in terms of general strategy: Let's try to merge #1410 first -- then we can simplify #1331 and merge that next -- and then we can merge dev into this PR, which will simplify the changes here. What do you think? Might be worth discussing the differences in Prettier configuration between this branch and #1331 -- if you think we should strip down the settings in #1331 to match, I'm happy to make that adjustment! |
That sounds like a sensible order to me - this branch is mostly in case we want to tinker with any settings. I've commented in #1331 re stripping the settings down. Re your workaround, I'm not too worried about these few style changes being included (I was pleasantly surprised there were so few) and I like the idea of the changes being in the same PR as the config changes because, as you say, it illustrates how the change affected things. We'll likely also get similar unrelated changes whenever files are worked on in future if we're going to accept import order optimization. |
|
As for the git action in general, I've re-checked what it runs and it just runs Prettier over everything, which isn't really linting the code. I think a preferable action would be to run ESLint over the code, and if errors are found then it highlights to us that changes are needed before a PR can be merged. This can be fed back to the developer who can then update the PR. I'm happy to have a separate action called something like 'Tidy', to run Prettier as well though, in case any contributors don't use it. |
|
@LlGC-jop, I have no objection to refining/renaming npm scripts and GitHub Actions to make things more logical. I think it's just a question of what order to do things in -- I would prefer to implement all of this in discrete steps rather than one big PR that does everything at once. But maybe as we discuss and make decisions, we can split parts of the work here off into separate PRs that can be independently merged, until we reduce this one down to something small enough to merge on its own, and then we're done! |
|
This branch seems to have become a little messy, probably due to it being based on I'll leave this here for now to serve as a reference though and have converted to a draft so it can't accidentally get merged. |
#1379
Merging POC branch into identically named branch in upstream to keep dev clean for now:
The config file changes seem to have affected the 'linting' that happens when github runs actions.
No ts files were changed as part of the work, so it should probably be discussed whether we want automatic formatting/fixing of files to happen or whether these should be run manually or only in the dev's IDE, so only files that were worked on show in commits/PRs.