Skip to content

feat: lint option for javascript template #505

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

Merged
merged 4 commits into from
Jun 24, 2022

Conversation

Mazuh
Copy link
Contributor

@Mazuh Mazuh commented Jun 13, 2022

resolves #363

Checklist

for typescript, see: #512

@Mazuh Mazuh marked this pull request as draft June 13, 2022 13:35
@Mazuh Mazuh changed the title draft: lint option feat: lint option for javascript template Jun 13, 2022
@Mazuh Mazuh force-pushed the feat/lint-option branch from 10b3bf9 to c6858f9 Compare June 13, 2022 14:31
@Mazuh Mazuh marked this pull request as ready for review June 13, 2022 14:32
@Mazuh Mazuh force-pushed the feat/lint-option branch from c6858f9 to e485b20 Compare June 14, 2022 08:44
@Mazuh Mazuh requested a review from mcollina June 14, 2022 08:44
@Mazuh Mazuh force-pushed the feat/lint-option branch from e485b20 to ee78eb2 Compare June 14, 2022 08:46
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

I would change the option to lint which can be extendable in future.

@Mazuh
Copy link
Contributor Author

Mazuh commented Jun 14, 2022

@climba03003, that's up to you folks to decide, but...

IMHO people that need something beyond the standard linter will probably do that with their own custom *rc files (.prettierc, .eslintrc), each one requiring extra dependencies (every eslint project has tons of different plugins listed on devdependencies), at least that's my experience, not to mention that eslint/prettier CLIs are already very customizable by themselves.

so, when I choose the boolean/binary approach, it was by design trying to prevent the cli of being redundant with eslint/prettier clis, prevent any kind of long discussion regarding code styles whenever I new one would be introduced via issue/PR, prevent the responsibility of maintaining any new linter set up-to-date forever and keep scope reasonably non-opinionated yet pragmatic about it -- tldr: I tried to avoid a lot of headache for maintainers.

anyway, that's a design choice, not really my decision to take, I'm available to make any changes, let me know what the rest of the reviews think and what should I do to get this merged.

@climba03003
Copy link
Member

IMHO people that need something beyond the standard linter will probably do that with their own custom *rc files (.prettierc, .eslintrc), each one requiring extra dependencies (every eslint project has tons of different plugins listed on devdependencies), at least that's my experience, not to mention that eslint/prettier CLIs are already very customizable by themselves.

It is a string option which mean it can be extended like --lint=standard, --lint=eslint, etc.

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

tldr: I tried to avoid a lot of headache for maintainers.

+1

I must say that it would be awesome to think of a "pluggable" cli (or a yeomen generator? 🤔 )

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@climba03003
Copy link
Member

climba03003 commented Jun 15, 2022

I must say that it would be awesome to think of a "pluggable" cli (or a yeomen generator? 🤔 )

@Eomm I recommend to check this PR #491 which is interactive and provide more option than the current one.
And I need some help on the Github Actions problem.

@Mazuh
Copy link
Contributor Author

Mazuh commented Jun 17, 2022

@climba03003 @Eomm any feedback on this? should I assume that this PR should be closed (it looks like the issue is being worked on in another PR or something)?

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

LGTM when ci green

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@simoneb
Copy link
Contributor

simoneb commented Jun 20, 2022

@Mazuh let's figure out the windows problem first

@Mazuh Mazuh force-pushed the feat/lint-option branch from b1eb752 to f0dae38 Compare June 21, 2022 11:04
removing directories with .sync is limited on windows
so I refactored my test to always use an available folder
instead of concurring with the existing one
@Mazuh Mazuh force-pushed the feat/lint-option branch from f0dae38 to 8a804c4 Compare June 21, 2022 11:10
@Mazuh Mazuh marked this pull request as draft June 21, 2022 11:13
@Mazuh Mazuh marked this pull request as ready for review June 21, 2022 11:19
@Mazuh
Copy link
Contributor Author

Mazuh commented Jun 21, 2022

it was indeed just a testing issue, the rimraf.sync(...) that runs before each test behaves a little bit differently on windows, all others test cases were running fine but my little one stepped on a race condition and I was trying to generate the project in a busy directory.

now I have my ubuntu and my windows with npm t returning 0 (ok), so the same should happen with the pipeline, I'm going back to my macbook to confirm that everything is ok there too.

Captura de tela 2022-06-21 081851


maybe it's worth to create an issue to refactor the test suite in another PR to use the async version of rimraf instead, but I'm not sure if this will also get in the middle of the big refactoring in progress on another PR.

@Mazuh
Copy link
Contributor Author

Mazuh commented Jun 21, 2022

Can one of you guys merge it, please? It seems all good now.

@mcollina
Copy link
Member

@simoneb you good to land this?

@simoneb
Copy link
Contributor

simoneb commented Jun 23, 2022

@mcollina yes LGTM

@mcollina mcollina merged commit 9214773 into fastify:master Jun 24, 2022
@Mazuh Mazuh deleted the feat/lint-option branch June 27, 2022 14:41
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.

Add minimal eslint and Prettier configurations
5 participants