Skip to content

Re-style the javascript for gulpfile#3912

Merged
djencks merged 2 commits intoapache:masterfrom
AemieJ:gulpfile-restyle
Jun 16, 2020
Merged

Re-style the javascript for gulpfile#3912
djencks merged 2 commits intoapache:masterfrom
AemieJ:gulpfile-restyle

Conversation

@AemieJ
Copy link
Contributor

@AemieJ AemieJ commented Jun 14, 2020

  • As discussed in PR#3879, the basic syntax and styling were overlooked for the gulpfile.js. I have replaced the usage of == with === and != with !== to create a strict comparison.
  • Apart from that, I observed that there were statements that included semicolons and a few that didn't hence to bring consistency, I included semicolons for each statement.

@djencks
Copy link
Contributor

djencks commented Jun 14, 2020

Thanks for looking into this, this is definitely an improvement.

I'm used to and prefer no semicolon line ends.

I'm used to projects enforcing style with ESLint and an .eslintrc file, and providing lint and lint-fix scripts in package.json.

The .eslintrc files I use are

{
  "extends": "standard",
  "rules": {
    "arrow-parens": ["error", "always"],
    "comma-dangle": ["error", {
      "arrays": "always-multiline",
      "objects": "always-multiline",
      "imports": "always-multiline",
      "exports": "always-multiline"
    }],
    "max-len": ["error", {
      "code": 120,
      "ignoreStrings": true,
      "ignoreUrls": true,
      "ignoreTemplateLiterals": true
    }],
    "spaced-comment": "off"
  }
}

and the bits of package.json

  "scripts": {
    "lint": "eslint lib/**/**.js test/**/**.js",
    "lint-fix": "eslint lib/**/**.js test/**/**.js --fix"
  },
  "devDependencies": {
    "eslint": "~6.7",
    "eslint-config-standard": "~14.1",
    "eslint-plugin-import": "~2.19",
    "eslint-plugin-node": "~10.0",
    "eslint-plugin-promise": "~4.2",
    "eslint-plugin-standard": "~4.0",
    "prettier-eslint-cli": "~5.0"
  }

I'm not sure if all the dependencies are needed.

What do you think about adding these scripts?

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 14, 2020

@djencks I was planning to add the similar formatting eslint and prettier styling as that in camel-website. I think the code you showed is similar to that.

@djencks
Copy link
Contributor

djencks commented Jun 14, 2020

Great! Mine is derived from Antora so is very similar.

@AemieJ AemieJ force-pushed the gulpfile-restyle branch from 867f587 to 1b94247 Compare June 14, 2020 16:21
@djencks
Copy link
Contributor

djencks commented Jun 15, 2020

I think it would be better to have this in the .eslintrc file:

/eslint semi: ["error", "never"]/

Can we remove the unused import here instead of suppressing the warning?

/* eslint-disable-next-line no-unused-vars */

Unfortunately there's a merge conflict now, can you rebase on master? I think it might be clearer (if you can figure out how :-) to have the added lint-config files in one commit and the effect of running lint in a separate commit. I've found this useful for splitting commits: https://hisham.hm/2019/02/12/splitting-a-git-commit-into-one-commit-per-file/

@djencks
Copy link
Contributor

djencks commented Jun 15, 2020

Also, I prefer separate lint and lint-fix scripts. If lint changes the source, you have to run git status to find out if anything changed, and lint can't be run to fail the build if there are lint errors.

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 15, 2020

@djencks I thought to remove it but I wanted to check if there was any importance to include it in the first place. That's why I suppressed the warning for the meanwhile.

@AemieJ AemieJ force-pushed the gulpfile-restyle branch from 1b94247 to e2e136e Compare June 15, 2020 19:07
Copy link
Member

@zregvart zregvart left a comment

Choose a reason for hiding this comment

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

Seems that we have one set of files missing here.

@AemieJ AemieJ force-pushed the gulpfile-restyle branch from e2e136e to 8234ae6 Compare June 15, 2020 20:13
@AemieJ AemieJ force-pushed the gulpfile-restyle branch from 8234ae6 to d6ce8f2 Compare June 16, 2020 04:45
@djencks djencks merged commit 2c5e9ff into apache:master Jun 16, 2020
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.

5 participants