Skip to content

Add linting and formatting#79

Merged
jamesrweb merged 28 commits intoP5-wrapper:masterfrom
yevdyko:style/add-linting-and-formatting
Apr 9, 2021
Merged

Add linting and formatting#79
jamesrweb merged 28 commits intoP5-wrapper:masterfrom
yevdyko:style/add-linting-and-formatting

Conversation

@yevdyko
Copy link
Contributor

@yevdyko yevdyko commented Mar 28, 2021

This MR adds linting and formatting to simplify the maintenance of the library component.

What was changed?

  • Add Prettier and its config for formatting.
  • Format component using Prettier.
  • Add pre-commit git hook that formats staged files.
  • Add ESLint packages for typescript as a linter.
  • Add ESLint config.
  • Add React specific linting rules for ESLint.
  • Add packages to integrate ESLint with Prettier rules.
  • Add CI Github action workflow.
  • Add documentation about ESLint and Prettier configurations.
  • Format the whole project using prettier.
  • Remove unused styles.
  • Fix linting for example js files.

@jamesrweb jamesrweb self-requested a review March 28, 2021 17:02
Copy link
Member

@jamesrweb jamesrweb left a comment

Choose a reason for hiding this comment

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

@yevdyko Looks good to me, some minor documentation additions and workflow updates to change but otherwise a really nice PR so far!

@yevdyko yevdyko changed the title Add Linting and Formatting Add linting and formatting Mar 30, 2021
@yevdyko yevdyko requested a review from jamesrweb April 1, 2021 22:21
Copy link
Member

@jamesrweb jamesrweb left a comment

Choose a reason for hiding this comment

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

@yevdyko Can you test my changes please?

I updated the CI and removed the git hooks because the CI is where we should be automating the integration of new code. Please check that I removed all non-necessary dependencies and that the CI workflow makes sense. On a sidenote, you mentioned extra commits, this would only happen if changes occur, not otherwise and so if linting and formatting don't change something, nothing is commited. The CI just helps the developer to develop and the CI to integrate so that the developer doesn't need to worry about code verification, formatting or stylistic issues and can write code how they wish and we format it to our needs, so long as it is reviewed / tested then it makes life easier for everyone in my experience plus it is what CI is for anyway.

Happy to answer any open questions and really happy with your work, this is really good stuff my friend! 👍🏻

@jamesrweb jamesrweb dismissed their stale review April 3, 2021 17:26

New review completed.

@yevdyko
Copy link
Contributor Author

yevdyko commented Apr 3, 2021

@jamesrweb I don't mind having a formatting step in the CI, it's more a matter of taste :) Also, if it's inconvenient, we can always change the approach we take.

After you specified the master branch as a trigger for the push in the CI I can't test how it works together :)

Please add also changes in the package-lock.json file. Committed the updated lock file.

Thanks for the feedback and support!

@yevdyko yevdyko requested a review from jamesrweb April 3, 2021 21:43
@jamesrweb
Copy link
Member

@jamesrweb I don't mind having a formatting step in the CI, it's more a matter of taste :) Also, if it's inconvenient, we can always change the approach we take.

After you specified the master branch as a trigger for the push in the CI I can't test how it works together :)

Please add also changes in the package-lock.json file. Committed the updated lock file.

Thanks for the feedback and support!

I don't know why it does that and I cannot update the settings but the reason master is defined is because we only want the CI script to run for PR's into master or (if admin rights exist) pushed into master occur. If we had it run on PR A going into PR B which is targeted at master then in this case only PR B would run CI since A wouldn't directly need to since it wouldn't affect the main codebase. Does that make more sense? I amn't an expert so to say with GitHub actions so maybe I am missing something but I have found this flow useful in other personal projects but open to changes if we find a better way.

Copy link
Member

@jamesrweb jamesrweb left a comment

Choose a reason for hiding this comment

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

Only one thing left and then LGTM!

@yevdyko
Copy link
Contributor Author

yevdyko commented Apr 8, 2021

I don't know why it does that and I cannot update the settings but the reason master is defined is because we only want the CI script to run for PR's into master or (if admin rights exist) pushed into master occur. If we had it run on PR A going into PR B which is targeted at master then in this case only PR B would run CI since A wouldn't directly need to since it wouldn't affect the main codebase. Does that make more sense? I amn't an expert so to say with GitHub actions so maybe I am missing something but I have found this flow useful in other personal projects but open to changes if we find a better way.

Yes, I don't mind, and let's stick to this flow. Thanks for adding a step with cache npm packages! 🤖

@yevdyko yevdyko requested a review from jamesrweb April 9, 2021 11:02
@jamesrweb jamesrweb merged commit 2c33a12 into P5-wrapper:master Apr 9, 2021
@yevdyko yevdyko deleted the style/add-linting-and-formatting branch April 9, 2021 14:23
jamesrweb added a commit that referenced this pull request Aug 15, 2022
* Add prettier

* Add prettier config

* Format component using prettier

* Add lint-staged

* Add lint-staged config

* Add simple-git-hooks

* Add pre-commit git hook

* Add eslint packages for typescript

* Add eslint config

* Add react specific linting rules for eslint

* Update eslint config with react plugin preset

* Add packages to integrate eslint with prettier rules

* Update eslint config with recommended prettier setup

* Avoid using trailing commas

* Add ci github action workflow

* Add documentation about eslint configuration

* Add documentation about prettier configuration

* Format the whole project using prettier

* Actualize ignored files

* Remove unused styles

* Extend eslint config to use with js files

* Fix linting for example js files

* Update ci.yml

* Update package.json

* Update package-lock.json file after removing packages

* Move testing to the separate job in CI

* Format JSON files as well

* Format README file using Prettier

Co-authored-by: James Robb <47126579+jamesrweb@users.noreply.github.com>
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