-
Notifications
You must be signed in to change notification settings - Fork 5
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/pre commit #175
feature/pre commit #175
Conversation
Mostly added TODOs
To get started I made a couple of commits on The only rule I've set is 4 spaces for indentation, which (mostly) matches what we've been doing. As a side note, prettier's philosophy is to not be very configurable, so if we find anything we hate that we can't configure, we should can it sooner rather than later. @TrevorBurgoyne @csolbs24 please complain as much as possible - I'd prefer to get a good set of rules now so we don't need massive diffs down the road. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall i think the rules look pretty g. eslint really loves const lol, tho i didn't see any obvious spots where it would affect anything negatively.
ive used husky in other projects and it works well. it can also support running tests on commit if we ever wanna do that.
doesn't look like anything enforces JSDoc for functions. i think thats probably something we should enforce, even if it means a lot of initial work.
also, having an import sorter would be kinda nice.
i think eslint has both an import sorter and a jsdoc requirement option so id say we turn both those on. maybe the jsdoc can just be a warning for now?
@TrevorBurgoyne I've finished resolving ESLint issues. I left a lot of TODOs, a lot of |
Yeah i have to scroll down far enough on this pr as is, id say split those out for later 👍 |
I added a note on #103. There aren't any other PRs that look like they're close to merging, so I'll get the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very close, a couple notes. also i think we should add a GHA as a linting check to make sure ppl actually have their pre commit hooks setup correctly
Co-authored-by: Trevor Burgoyne <82477095+TrevorBurgoyne@users.noreply.github.com>
Co-authored-by: Trevor Burgoyne <82477095+TrevorBurgoyne@users.noreply.github.com>
Opened #192 to track this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yipee 🛩️
Formatting and Linting Hooks
Description
Added:
I expect this might remain open for a bit longer than other PRs, so I'm going to forego constantly merging the latest
main
until we've settled on a good set of rules.PR Checklist
package.json
has been bumped since last releasepackage.json
andsrc/version.js
npm install
andnpm run build
AFTER bumping the version numberapi_spec.md
)changelog.md
Breaking API Changes
nope