chore: set up initial actions & docs #1
Conversation
|
just throwing this previous conversation here nodejs/release-cloudflare-worker#132 - it's not a blocker, but i honestly dont know how to manage this sort of fragmentation across the project - perhaps it doesnt matter |
|
I think some fragmentation is fine. I think it's more important to keep standards similar. How they are enforced is an implementation detail, and the existing modus operandi of the project is, without a compelling reason to the contrary, that's author's choice. ESLint being "in the family" is maybe a compelling reason, but i think not a clear-cut one. I don't have much preference though. ESLint has been fine when I've used it before. I only tossed in biome here because of the reasons Augustin mentioned. |
|
Note, you put the MIT license in |
Co-authored-by: Augustin Mauroy <[email protected]>
| @@ -0,0 +1,18 @@ | |||
| # Contributing | |||
There was a problem hiding this comment.
Need to add code snippets about how to run test and how to write a test.
Take example on nodejs.org repo we had done huge work to have something really explicit
There was a problem hiding this comment.
Good idea!
This PR isn't finished btw. Just as much as I could get done last night. But it's ready for input/discussion 🙂
|
Should we have commit hooks ? |
.github/workflows/ci.yml
Outdated
| node-version: ${{ matrix.node-version }} | ||
| cache: 'npm' | ||
| - run: npm ci | ||
| - run: npm run lint |
There was a problem hiding this comment.
this should be done in sub steps ? I'm no sure.
@bmuenzenmeyer is more of an expert on this than I am
| "javascript.updateImportsOnFileMove.enabled": "always", | ||
| "typescript.updateImportsOnFileMove.enabled": "always" |
There was a problem hiding this comment.
I think we need to set an additional property so that VS Code doesn't change d.ts → .js when it updates imports. I don't recall what the config prop is though.
There was a problem hiding this comment.
I've no idea, but I think this behaviour is gone now, but not for sure.
There was a problem hiding this comment.
Definitely not gone. It happened to me like yesterday.
| - Do `node --run lint:fix` to fix linting issues | ||
| - Do `node --run format:fix` to format code | ||
| - Do `node --run test:types` to check types | ||
| - Do `node --run test` to run tests (it's should pass 😃) |
There was a problem hiding this comment.
Should we create a superset of these so users just have to do 1 thing? Ex node --run pre-commit
| "lint": "biome lint ./", | ||
| "lint:fix": "biome lint --fix ./", | ||
| "format": "biome format ./", |
There was a problem hiding this comment.
What's the difference between lint and format?
There was a problem hiding this comment.
if you take look to biome rules set there are two part linter and formater
- linter is a code analyser
- formater is a code formater
IDK if you get the idea put it's two separate things
.github/workflows/ci.yml
Outdated
| - run: npm run lint | ||
| - run: npm run test:types |
There was a problem hiding this comment.
Same: Should these be switched to node --run …?
Also, should we add format?
There was a problem hiding this comment.
Also, should we add format?
We can use a separate step using the biome action that include a reporter
should we include or exclude this file ? |
Co-authored-by: Jacob Smith <[email protected]>
I think no: those can get hella annoying. If a user wants to commit code that will fail, that doesn't hurt anything, so let's not get in the way—maybe they have a good reason (like sharing something incomplete). |
I think include |
No description provided.