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

Setup improvements #98

Draft
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Draft

Setup improvements #98

wants to merge 4 commits into from

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Mar 27, 2023

This PR

  • Updates README
  • Add pre-commit linter hooks

Screenshots

lint:js

image

lint:css

image

@renintw renintw force-pushed the enhance/setup-improvements branch from 0894b7e to 874d184 Compare March 27, 2023 19:02
@renintw renintw force-pushed the enhance/setup-improvements branch from 874d184 to d781277 Compare March 27, 2023 19:03
@renintw renintw requested a review from iandunn March 27, 2023 19:05
@renintw renintw self-assigned this Mar 27, 2023
@renintw renintw added the enhancement New feature or request label Mar 27, 2023
Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

This looks good 👍🏻

It might be good to have the linter run on PRs too, but this is a good first step.

README.md Outdated Show resolved Hide resolved
settings/package.json Show resolved Hide resolved
@@ -0,0 +1,5 @@
#!/usr/bin/env sh
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this would be better as a pre-push hook? I often make lots of WIP commits locally, and it can be annoying for the linter to run on those, since they're not intended to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to support pre-push, but the usage for the new version has changed, and pre-push doesn't seem to work anymore 🤔 I've tried for a while and also checked if others have encountered the same issue, but no good news. Let me do more research to see if I can find a solution.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it's not a huge deal if we keep it pre-commit, especially if later on we add an action to do it on push.

@renintw renintw force-pushed the enhance/setup-improvements branch 4 times, most recently from bb77506 to 8954ae1 Compare March 27, 2023 22:54
@renintw
Copy link
Contributor Author

renintw commented Mar 27, 2023

It might be good to have the linter run on PRs too, but this is a good first step.

I'll add it when I circle back to this PR later. I'll proceed to other higher-priority tasks at the moment, pre-push doesn't seem to work somehow on my side.

@renintw renintw requested review from iandunn and removed request for iandunn March 27, 2023 23:05
@renintw renintw marked this pull request as draft March 27, 2023 23:06
@iandunn
Copy link
Member

iandunn commented Mar 28, 2023

That makes sense 👍🏻 The GH action isn't a high priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants