-
Notifications
You must be signed in to change notification settings - Fork 597
Unifies TypeScript packages and command names #3195
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
base: tyler/ts-module-test
Are you sure you want to change the base?
Conversation
@@ -8,7 +8,7 @@ on: | |||
merge_group: | |||
|
|||
jobs: | |||
compile-and-test: | |||
build-and-test: |
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.
FYI I believe this job name is used in the required status checks, so we will have to update the required name after this merges
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.
Can you add that note into the PR description somewhere so it's a bit easier to remember?
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.
Ah okay, yes I can
docs/docs/appendix.md
Outdated
@@ -6,15 +6,15 @@ For each table containing an `#[auto_inc]` column, SpacetimeDB creates a sequenc | |||
|
|||
### How It Works | |||
|
|||
* Sequences in SpacetimeDB use Rust’s `i128` integer type. |
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.
If these docs tweaks are important to you, can you please move them into a separate PR? They seem entirely unrelated to the (meaningfully large) actual goal of this PR.
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.
I typically do something like:
git checkout -b new-branch origin/master
git checkout old-branch -- file1 file2 ...
git commit -am 'Moving files from other thing'
git push -u
gh pr create --web
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.
Ah is this because we started running pnpm format
on the docs? That definitely seems like a separate change, since the docs are entirely unrelated to our TypeScript bindings/SDK.
By default I would instead lean towards adding the docs to .prettierignore
.
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.
Yes, this is because I'm running the same pnpm format on all typescript directories (directories with package.json). I can exempt markdown from the formatting and add that in a separate PR if you'd like.
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.
I do think that eventually we should have docs under prettier so that we don't have non-substantive formatting changes to the docs causing diffs in the future.
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.
I've added **/*.md
to the docs .prettierignore
. Notably the cli-reference.md
file formatting is still changed because I moved the not portable sed
scripts into a node js script which rewrites the file with remark.
@@ -0,0 +1,99 @@ | |||
#!/usr/bin/env node |
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.
This script replaces the non-portable sed commands that were used to fix up the markdown.
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.
Approving the docs formatting + CI changes, with the caveat that I think none of them should actually be happening in this PR (because I don't see the docs directory as a typescript project, or at the very least not the same kind of typescript project as the ones that are related to actual SpacetimeDB implementation). But if I close my eyes and pretend that this PR is only making those changes, they seem good to me.
"unified": "^11.0.5", | ||
"unist-util-visit": "^5.0.0" | ||
}, | ||
"scripts": { | ||
"build": "tsc --project ./tsconfig.json", | ||
"build": "tsc --project ./tsconfig.json && pnpm fix-markdown && prettier --write docs/nav.js", |
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.
Should a formatting step really be done during build?
"build": "tsc --project ./tsconfig.json && pnpm fix-markdown && prettier --write docs/nav.js", | |
"build": "tsc --project ./tsconfig.json && pnpm fix-markdown", |
579f3f7
to
c62e7f6
Compare
…d root package.json, and standardized script commands
13b4996
to
4cca832
Compare
Description of Changes
This PR:
pnpm
commands to be the sameIMPORTANT! Once this PR merges we will need to change the
compile-and-test
required check tobuild-and-test
API and ABI breaking changes
No breaking changes.
Expected complexity level and risk
2 - It in principle doesn't change any code, but could affect deploy processes.
Testing