Skip to content

feat: generate lint on new project and run lint on update project #149

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.

Sign up for GitHub

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

Open
mtperesvx wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: generate lint on new project and run lint on update project #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f589f4
feat: generate lint on new project and run lint on update project
mtperesvx Jul 26, 2022
725b75f
Merge branch 'main' into feat-herbsWlint
mtperesvx Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/commands/new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ const questions = [
return val.toLowerCase()
},
},
{
type: 'confirm',
name: 'ESlint',
message: 'Do you want to initialize a ESlint on this project',
default: true,
},
{
type: 'confirm',
name: 'git',
Expand Down Expand Up @@ -136,6 +142,7 @@ const cmd = {

if (npmOptions.npmInstall === 'Yeah, please' || npmOptions.npmInstall === 'yes') {
await exec('npm install')
await exec('npx eslint \"**/*.{js,jsx}\" --fix')
Copy link
Member

@italojs italojs Aug 2, 2022

Choose a reason for hiding this comment

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

if we run it without an eslint file configured, it will ask to install it.
so, if I said before (line 69) that i don't want to use eslint on my project and it(npx eslint) installs the eslint when I generate the project, we will provide a bad DX for programmer

We need to run it only if the project use eslint (when generated or after it)

Copy link
Member

@dalssoft dalssoft Aug 7, 2022

Choose a reason for hiding this comment

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

since herbs update will run the lint command, this line is not necessary for when the user chooses Yeap. However, it is necessary when the user chooses No.

await exec('herbs update')
}
await checkNewVersion()
Expand Down
1 change: 1 addition & 0 deletions src/commands/update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const cmd = {
alias: ['u'],
run: async toolbox => {
const generators = (await generator(toolbox)).update
await exec('npx eslint \"**/*.{js,jsx}\" --fix')
Copy link
Member

@italojs italojs Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we need to check if exists an eslint file, if we run it without an eslint file, the npx eslit will ask to install and configure it.
it could generate a bad DX for the programmer
Screen Shot 2022-08-01 at 22 07 44

for (const layer of Object.keys(generators)) { await generators[layer]() }
toolbox.print.success('Project updated! 🤩')
}
Expand Down
1 change: 1 addition & 0 deletions src/generators/src/packagejson.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const optionalPackages = {
mysql: ['"@herbsjs/herbs2knex": "^1.5.2"', '"mysql2": "^2.3.3"'],
rest: ['"express": "^4.18.1"', '"cors": "^2.8.5"', '"@herbsjs/herbs2rest": "^3.0.1"'],
graphql: ['"graphql": "^16.5.0"', '"@herbsjs/herbs2gql": "^2.2.0"', '"apollo-server": "^3.8.2"','"apollo-server-express": "^3.8.2"', '"graphql-tools": "^8.2.12"', '"graphql-scalars": "^1.17.0"',]
eslint: ['"eslint": "^8.20.0"', '"eslint-config-standard": "^17.0.0"', '"eslint-plugin-import": "^2.26.0","eslint-plugin-n": "^15.2.4"','"eslint-plugin-promise": "^6.0.0"']
Copy link
Member

@italojs italojs Aug 2, 2022

Choose a reason for hiding this comment

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

we don't generate code using import, are you sure we need the eslint-plugin-import pkg?

}

const defaultOptions = (options) => {
Expand Down
15 changes: 15 additions & 0 deletions src/templates/.eslintrc.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module.exports = {
env: {
es2021: true,
node: true
},
Copy link
Member

@italojs italojs Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

why not just call the npx eslint after creating the project? so the developer could use the eslint CLI to generate it as they need/want to, we don't need to generate/manage it, just pass this job for eslint CLI
Screen Shot 2022-08-01 at 22 07 44

Copy link
Contributor Author

@mtperesvx mtperesvx Aug 5, 2022

Choose a reason for hiding this comment

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

Would it make sense to keep the lint --fix command as the responsibility of the herbs update command in this case?

Copy link
Member

@italojs italojs Aug 6, 2022

Choose a reason for hiding this comment

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

Only the lint, so we will warn the developer about the problems, run the lint --fix could be so much invasive for the developer, for some reason they may want to maintain the code with the "lint problems"

extends: [
'standard'
],
parserOptions: {
ecmaVersion: 'latest',
sourceType: 'module'
},
rules: {
}
}