Skip to content

Comments

feat(eslint-config): add node and jest support#6

Draft
JoshCoady wants to merge 1 commit intomainfrom
feature/node
Draft

feat(eslint-config): add node and jest support#6
JoshCoady wants to merge 1 commit intomainfrom
feature/node

Conversation

@JoshCoady
Copy link

Docs need to be updated.

@JoshCoady JoshCoady requested a review from paulmelero June 17, 2022 18:26
Copy link
Contributor

@paulmelero paulmelero left a comment

Choose a reason for hiding this comment

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

👏🏽 LGTM

As you said, we would need to update the README too.

Comment on lines +12 to +28
// common ones
// biblio
semi: ['error', 'never'],
quotes: [
'error',
'single',
{ avoidEscape: true, allowTemplateLiterals: true },
],

// wf
'no-debugger': process.env.NODE_ENV === 'production' ? 'error' : 'off',

// ydr

// bib-service-projects
'require-atomic-updates': 'warn',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that these were originally linked to specific projects still makes them suitable for node or jest environments, I would move them to the shared file 🙏🏽

Copy link
Author

Choose a reason for hiding this comment

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

should we just remove (or have as a goal to remove eventually) anything for specific projects and have those projects just override any of the default settings they want to be diff?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I didn't include 100% of their rules. Only the ones that I considered to be best practices. And also, semi and quotes match the prettier config. So we don't have issues using prettier-config and eslint-config together, which is what I'd recommend.

Copy link
Contributor

Choose a reason for hiding this comment

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

The references to different projects are left there only to indicate that we did a "merge" of all the different preferences everyone had. To highlight that it was done as democratically as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

After reviewing it again, these rules that were in the index.js file are all good for node.js, including the extended configurations.

Comment on lines +8 to +9
extends: ['plugin:jest/recommended'],
plugins: ['jest'],
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires us to have the eslint-plugin-jest dependency as well. And add it in the readme as a requirement alongside the other dependencies...

Comment on lines +4 to +6
'plugin:node/recommended',
],
plugins: ['node'],
Copy link
Contributor

Choose a reason for hiding this comment

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

@paulmelero
Copy link
Contributor

And sorry, I never pulled off continuing with #4

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.

2 participants