Skip to content
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

Feature/emulsify linting setup #111

Merged
merged 26 commits into from
Sep 23, 2020
Merged

Conversation

jani-koski
Copy link
Contributor

@jani-koski jani-koski commented Jun 15, 2020

https://wunder.atlassian.net/browse/PSW-1

  • A linting configuration that uses the same settings as the Emulsify theme, but I've taken out all the non-relevant things, such as any configuration that is related to React.
  • For unifying purposes, I changed all the config files to .rc with JSON syntax.
  • On the Emulsify theme, .stylelintrc-file is located in the Webpack folder, and there possibly is a practical reason for that. But at this point, as we don't even have Webpack on the repo, I just added the stylelint file to the root of the project.

@dbruvers dbruvers self-requested a review July 17, 2020 15:16
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.eslintrc Outdated
@@ -0,0 +1,22 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Two comments here:

  1. The config file name should be .estlintrc.json. From Eslint docs:
ESLint supports configuration files in several formats:

    JavaScript - use .eslintrc.js and export an object containing your configuration.
    ...
    JSON - use .eslintrc.json to define the configuration structure. ESLint's JSON files also allow JavaScript-style comments.
    Deprecated - use .eslintrc, which can be either JSON or YAML.
  1. When we run npm run lint from the project root then ESlint finds ./web/.eslintrc.json. We need to discuss whether it makes sense to have an additional .eslintrc.json file at the project root.

package.json Show resolved Hide resolved
.stylelintrc Outdated Show resolved Hide resolved
@jani-koski jani-koski self-assigned this Sep 9, 2020
Copy link
Contributor Author

@jani-koski jani-koski 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 say that things generally are looking pretty good, or at least something that we can start off with. I did a few more changes though:

  • Fixed style linting paths (lint:css) paths in package.json
  • Installed stylelint-prettier package. I noticed that we had not yet taken into account running prettier rules for css files. We had only disabled formating related rules, but not installed or configured a package which runs prettier as a stylelint rule; stylelint-prettier does exactly that.

Now, at this point someone should probably test that the linting setup works as desired:

  • Eslint runs in a way that formatting related rules are handled by prettier
  • Stylelint runs in away that formatting related rules are handled by prettier

Copy link
Member

@joshua-scott joshua-scott left a comment

Choose a reason for hiding this comment

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

Works well for me, I think this is a great starting point. Made one small suggestion.

.eslintrc.json Outdated
Comment on lines 13 to 16
"globals": {
"expect": true,
"it": true,
"describe": true,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding expect/it/describe as globals, we could instead add jest to env: "env": { "jest": true }

@dbruvers dbruvers merged commit b4907a5 into master Sep 23, 2020
@dbruvers dbruvers deleted the feature/emulsify-linting-setup branch September 23, 2020 11:13
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.

3 participants