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

chore: use prettier for lint #1362

Merged
merged 3 commits into from
Mar 25, 2020
Merged

chore: use prettier for lint #1362

merged 3 commits into from
Mar 25, 2020

Conversation

vhashimotoo
Copy link
Contributor

@vhashimotoo vhashimotoo commented Mar 24, 2020

Does what it says on the tin.

Also enabled lint for yaml files so every app is similar to others.

@vhashimotoo vhashimotoo changed the title lint: use prettier for lint chore: use prettier for lint Mar 24, 2020
@vhashimotoo vhashimotoo changed the title chore: use prettier for lint [wip] chore: use prettier for lint Mar 24, 2020
@vhashimotoo vhashimotoo changed the title [wip] chore: use prettier for lint chore: use prettier for lint Mar 25, 2020
@vhashimotoo vhashimotoo requested a review from a team March 25, 2020 18:04
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

Could we add a pre-commit linter hook here?

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

👍 for consistency. Agreed with Erick, we should add something like husky + lint-staged.

Comment on lines +19 to +20
npm ci
npm test
Copy link
Member

Choose a reason for hiding this comment

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

Maybe for another PR, but generally I've put npm ci and npm test in different run steps, but if you want to do it in one step:

Suggested change
npm ci
npm test
npm install-ci-test

Comment on lines +11 to +18
if: github['ref'] == 'refs/heads/master'
- name: Use Node.js 10.x
if: github['ref'] == 'refs/heads/master'
uses: actions/setup-node@v1
with:
node-version: 10.x
- name: npm install, build, and test
if: github['ref'] == 'refs/heads/master'
Copy link
Member

Choose a reason for hiding this comment

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

Again, maybe for another PR, but shouldn't the branch check be on the on: push instead of on each step?

on:
  push:
    branches:
      - master

@vhashimotoo
Copy link
Contributor Author

vhashimotoo commented Mar 25, 2020

It's pretty old config and I plan to update them to standard web infra config in refactor PR.

Added husky + lint-staged for pre-commit lint.

EDIT: Also we maybe need a request update the required checks.

@vhashimotoo vhashimotoo requested a review from malept March 25, 2020 18:44
@vhashimotoo vhashimotoo merged commit 33d06c9 into master Mar 25, 2020
@vhashimotoo vhashimotoo deleted the use-prettier branch March 25, 2020 19:25
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