-
Notifications
You must be signed in to change notification settings - Fork 0
Whitespace #16
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
Whitespace #16
Conversation
| } as ChelContractState, | ||
| serializedEncryptedData, | ||
| 0, | ||
| { [id]: key } | ||
| ) |
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.
Well, I suppose these types of changes aren't really needed either.
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 can't seem to find another way of doing this that isn't awkward (apparently, lines 76-78 could be collapsed into one, but that seems inconsistent and harder to read) and doesn't exceed the line length.
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.
No big deal either way I suppose. But note that prettier doesn't need to be run again.. And really we don't need to add it as a project dependency
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.
And really we don't need to add it as a project dependency
But it's not being added as a project dependency, is it?
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.
Indeed you're right 👍
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.
(Arguably there's a good reason to add it as a dependency, which should be a separate issue: apparently eslint is somewhat (*) able to enforce max line length rules, but it's not able to 'fix' them, so for DX it's slightly better to run eslint with prettier)
(*) Some things seem to escape it, see the chelonia.ts example below
| "test": "node --import 'data:text/javascript,import { register } from \"node:module\"; import { pathToFileURL } from \"node:url\"; register(\"ts-node/esm\", pathToFileURL(\"./\")); process.on(\"uncaughtException\", (e) => { console.error(\"ERROR\", e); throw e });' src/index.test.ts", | ||
| "build:esm": "node --import 'data:text/javascript,import { register } from \"node:module\"; import { pathToFileURL } from \"node:url\"; register(\"ts-node/esm\", pathToFileURL(\"./\")); process.on(\"uncaughtException\", (e) => { console.error(\"ERROR\", e); throw e });' buildHelper.ts esm", | ||
| "build:cjs": "node --import 'data:text/javascript,import { register } from \"node:module\"; import { pathToFileURL } from \"node:url\"; register(\"ts-node/esm\", pathToFileURL(\"./\")); process.on(\"uncaughtException\", (e) => { console.error(\"ERROR\", e); throw e });' buildHelper.ts cjs", |
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.
These aren't strictly speaking relevant to whitespace, but throwing is necessary in this case to propagate errors if they should occur.
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.
Also: Node 24 (well, since 22 apparently) natively supports TypeScript, so once we upgrade we can remove the dependency on ts-node.
taoeffect
left a comment
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.
Awesome!
No description provided.