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/pre commit #175

Merged
merged 51 commits into from
Oct 4, 2024
Merged

feature/pre commit #175

merged 51 commits into from
Oct 4, 2024

Conversation

joshua-dean
Copy link
Collaborator

@joshua-dean joshua-dean commented Sep 27, 2024

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

  • Merged latest main
  • Version number in package.json has been bumped since last release
  • Version numbers match between package package.json and src/version.js
  • Ran npm install and npm run build AFTER bumping the version number
  • Updated documentation if necessary (currently just in api_spec.md)
  • Added changes to changelog.md

Breaking API Changes

nope

@joshua-dean
Copy link
Collaborator Author

To get started I made a couple of commits on src/index.js and src/toolbox.ts, so we could see linting/formatting on both javascript and typescript files. I separated prettier, eslint complaints, and eslint auto-fixes (when applicable) into separate commits so you could inspect them separately.

The only rule I've set is 4 spaces for indentation, which (mostly) matches what we've been doing.
For many of the eslint complaints, I left TODOs if I was concerned about harming functionality; we can always fix those later.

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.

@joshua-dean joshua-dean marked this pull request as ready for review September 27, 2024 19:45
Copy link
Member

@TrevorBurgoyne TrevorBurgoyne left a 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?

src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/toolbox.ts Outdated Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
@joshua-dean
Copy link
Collaborator Author

@TrevorBurgoyne I've finished resolving ESLint issues.

I left a lot of TODOs, a lot of any types are related to #131 so aren't really worth chopping up now.
We currently don't have anything for import sorting or JSDoc/TSDoc enforcement. I can try to sort those out in this PR if we want, or add them to #103 and sort them out later.

@TrevorBurgoyne
Copy link
Member

I can try to sort those out in this PR if we want, or add them to #103 and sort them out later.

Yeah i have to scroll down far enough on this pr as is, id say split those out for later 👍

@joshua-dean
Copy link
Collaborator Author

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 main merged and get this up-to-date.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@TrevorBurgoyne TrevorBurgoyne left a 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

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
src/configuration.ts Show resolved Hide resolved
src/configuration.ts Outdated Show resolved Hide resolved
src/geometric_utils.ts Outdated Show resolved Hide resolved
src/geometric_utils.ts Outdated Show resolved Hide resolved
joshua-dean and others added 5 commits October 4, 2024 11:29
Co-authored-by: Trevor Burgoyne <82477095+TrevorBurgoyne@users.noreply.github.com>
Co-authored-by: Trevor Burgoyne <82477095+TrevorBurgoyne@users.noreply.github.com>
@joshua-dean
Copy link
Collaborator Author

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

Opened #192 to track this

Copy link
Member

@TrevorBurgoyne TrevorBurgoyne left a comment

Choose a reason for hiding this comment

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

yipee 🛩️

@joshua-dean joshua-dean merged commit ab4f4d0 into main Oct 4, 2024
@joshua-dean joshua-dean deleted the feature/pre-commit branch October 4, 2024 17:47
@joshua-dean joshua-dean mentioned this pull request Oct 11, 2024
6 tasks
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