Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Extend linting to types/ directory #2488

Closed
gwhitney opened this issue Mar 17, 2022 · 7 comments
Closed

Extend linting to types/ directory #2488

gwhitney opened this issue Mar 17, 2022 · 7 comments

Comments

@gwhitney
Copy link
Collaborator

Set up linting for the typescript files!_ Or maybe I just have it configured wrong locally.

Originally posted by @ChristopherChudzicki in #2432 (comment)

No, I can confirm no linting is happening for me as well, and it would be very useful, vis-a-vis the last commit to #2432.

@ChristopherChudzicki
Copy link
Contributor

I'm happy to give this a shot this weekend.

@josdejong
Copy link
Owner

Thanks @ChristopherChudzicki 👍

@ChristopherChudzicki
Copy link
Contributor

ChristopherChudzicki commented Mar 20, 2022

I just spent a little while looking into this.

The /types directory does contain a tslint file and the root package.json requires dtslint, but neither of these really seems to be used. My understanding of the current "linting of /types" situation is:

So I do not think we should continue using dtslint or tslint.

Can we use StandardJS?

tldr: Yes, via command line. But IDE tooling seems pretty limited, at least in VSCode.

It would be nice to use StandardJS, since that's what he rest of the repo uses. There is typescript support for StandardJS via ts-standard.

  • it works via command line and has auto fix for most errors

  • tooling seems fairly limited. For example, there are two StandardJS VS Code plugins: an old one which recommends using this newer one instead.

    I can't get the new one to display linting errors in with ts-standard at all. I can get the old one to integrate with VS Code, but only if I move tsconfig.json to project root. The CLI supports a --project option to specify tsconfig location, but this doesn't seem to work in the editor plugin.

So... ESLint + Prettier?

They certainly have very good tooling. In the original linting discussion #1110, I saw low-configuration as a priority. Prettier is definitely low-configuration, and ESLint can be with plugins like plugin:@typescript-eslint/recommended.

I do not really like the idea of a separate linting system just for the TS files, but it's certainly possible, and IMO good tooling is very important for linters.

@josdejong cc @gwhitney How objectionable do you find the idea of using something like ESLint + Prettier for linting the type declarations, with the goal that formatting etc should be similar to that of the source code? (e.g., no semicolons, which is currently not true of the ts files).

@gwhitney
Copy link
Collaborator Author

I don't have an oar in this river. Anything someone else sets up that works fine is OK with me, and I have no particular views on how the TypeScript files are formatted, since after all there are only two of them in this repo. So I defer to you and Jos.

@josdejong
Copy link
Owner

Thanks @ChristopherChudzicki for investigating this. Your understanding of the current situation is correct.

How objectionable do you find the idea of using something like ESLint + Prettier for linting the type declarations

I have no preference over either StandardJS or ESLint+Prettier as long as they do the job. It makes sense to follow what is most mainstream, which typically means good IDE and tooling support. So that would be ESLint+Prettier nowadays I think.

I do think we should have a single linting setup for the whole repo though, and not something different for the pain JS vs the TypeScript definitions. Would it make sense to completely replace StandardJS with ESLint+Prettier and configure it correctly (and consistent) for both JS and TS?

One caveat when we would replace StandardJS: if that result in a (slightly) differing code style, it could result in a huge amount of changes throughout the whole the code base when formatting it. In that case we need to be a bit careful: that can give merge conflicts with open PR's etc. If we can avoid that it would be nice.

@ChristopherChudzicki
Copy link
Contributor

@josdejong Well, I was hoping to get to this sooner, but I just put up #2541 for this.

@gwhitney
Copy link
Collaborator Author

gwhitney commented May 4, 2022

This appears to be resolved in develop with the merge of #2544; closing.

@gwhitney gwhitney closed this as completed May 4, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants