-
Notifications
You must be signed in to change notification settings - Fork 324
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
Fix ESLint errors, add some docs #11339
base: develop
Are you sure you want to change the base?
Conversation
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.
Some old notes must be restored.
Also, having fixed it, we may add --max-warnings=0
parameter to lint command in every package. Or just have one lint script, in the root 'package.json' with this option.
@@ -6,6 +6,7 @@ import * as fsSync from 'node:fs' | |||
import * as fs from 'node:fs/promises' | |||
import * as os from 'node:os' | |||
import * as path from 'node:path' | |||
import * as process from 'node:process' |
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.
Is this import used?
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.
technically yes, process
is normally an implicit global in node.js but i don't think it hurts to make it an explicit import
/** | ||
* | ||
*/ |
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.
Please, mass find-replace these to single-line /** TODO: Add docs */
/** | ||
* Create a {@link WSSharedDoc}. | ||
*/ |
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.
A nitpick, but our jsdoc lints allow making single-line docs, and I think it looks better for trivial explanations.
/** | |
* Create a {@link WSSharedDoc}. | |
*/ | |
/** Create a {@link WSSharedDoc} */ |
It's ok to go anyway, I may change such docs occasionally.
getConcrete(child: RawNodeChild): NodeChild<Ast> | NodeChild<Token> { | ||
if (isTokenId(child.node)) | ||
return { whitespace: child.whitespace, node: this.getToken(child.node) } | ||
else return { whitespace: child.whitespace, node: this.get(child.node) } | ||
} | ||
|
||
/** @internal Copy a node into the module, if it is bound to a different module. */ | ||
/** @internal */ |
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.
Why we removed docs here?
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.
eslint autofix broke it 😭
/** | ||
* | ||
*/ | ||
copyIfForeign<T extends MutableAst>(ast: Owned<T> | undefined): Owned<T> | undefined { |
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.
There actually were docs for this function, just before overloads (and our lint plugin does not recognize 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.
true, i've added comments saying "this is the implementation" elsewhere just to make the plugin happy
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 think the proper solution would be to just put move the docs before implementation. Lint does not require those for overloads.
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 totally would have done that if i could! the issue with that is, then the docs seem to disappear from hover tooltips if i do that (because there no longer are docs attached to the overloads)
@@ -469,7 +472,7 @@ export function print(ast: Ast): PrintedSource { | |||
return { info, code } | |||
} | |||
|
|||
/** @internal Used by `Ast.printSubtree`. Note that some AST types have overrides. */ | |||
/** @internal */ |
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.
Another useful comments removed...
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 wonder if we could instead make something private in this file?
allowDefaultProject: [ | ||
'app/ydoc-server/vitest.config.ts', | ||
'app/ydoc-shared/vitest.config.ts', | ||
], |
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.
Why do we need this here, and not in other packages?
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 have no clue honestly, maybe vitest.config.ts
is ignored by eslint in other packages? it's definitely quite odd
Pull Request Description
Important Notes
None
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,or the Snowflake database integration, a run of the Extra Tests has been scheduled.
If applicable, it is suggested to paste a link to a successful run of the Extra Tests.