[atom] Use inline documentation for types in grammar-registry.d.ts#56369
[atom] Use inline documentation for types in grammar-registry.d.ts#56369icecream17 wants to merge 7 commits intoDefinitelyTyped:masterfrom
grammar-registry.d.ts#56369Conversation
See DefinitelyTyped#56297 I didn't remove undocumented methods or add deprecated methods for now. For `addInjectionPoint` I'm not really sure what "syntax node" refers to. See atom/atom#23019 atom/atom@37a3ae1 might help, although the file has changed since then to only use 1 param: https://github.com/atom/atom/blame/v1.40.0/spec/tree-sitter-language-mode-spec.js. I can't find anything with "node.child". For `getAssignedLanguageId`, if you look in the code, it's just a `Map#get` call. Which could be undefined. Now if you look even closer, `assignLanguageMode` and `assignGrammar` also set values to null: https://github.com/atom/atom/blob/v1.40.0/src/grammar-registry.js#L137 So `getAssignedLanguageId` actually returns `undefined | null | string`. But for now, I'm sticking to the documentation in the code `readGrammar` either calls the callback with `null, Grammar` or `error`. I guess the docs treat `undefined` as equivalent to `null`. But I'm changing the docs anyway. `getGrammars` docs say "options" but the param is actually called "params".
|
I just figured out what SyntaxNode is: https://github.com/tree-sitter/tree-sitter/blob/0e26fbe5e6d74b178b4f4b3ac318e2851bb0019b/lib/binding_web/tree-sitter-web.d.ts#L57
|
lierdakil
left a comment
There was a problem hiding this comment.
Sorry I've let this sit for a month. I'm sure you're not interested in excuses, but I've had some annoying health problems for the past three weeks or so, which made it really hard to do much beyond necessity.
Anyway, this overall looks good to me, but any is really a misfeature, I strongly believe we should avoid that.
types/atom/src/grammar-registry.d.ts
Outdated
| @@ -1,5 +1,12 @@ | |||
| import { Disposable, Grammar, GrammarToken, TextBuffer } from '../index'; | |||
|
|
|||
| type SyntaxNode = any; | |||
There was a problem hiding this comment.
CI complains about this, and I really don't like this either, because any opens a gaping hole in the type system, essentially, anything with any isn't typechecked.
I think we can do better: define an empty interface SyntaxNode. Consumers then can extend it in case they need to do something with syntax nodes beyond passing them around.
|
Also, remember that we not only have tree-sitter grammars but textmate grammars too. |
|
|
When applying the empty interface suggestion, I ran into the ci lint error:
```ERROR: 3:11 no-empty-interface An empty interface is equivalent to `{}`.```
|
|
Considering Atom will be sunset by the end of the year, I wonder if this PR is still relevant. I've personally switched to vscode, so I don't have a stake in this anymore, but if you want to finish up this PR, I'll approve it once CI stops complaining. |
npm test <package to test>.Select one of these and delete the others:
If changing an existing definition:
Details
Fixes the problem mentioned here: #56297
I didn't remove undocumented methods or add deprecated methods for now. (Although if it should be otherwise let me know.)
Sometimes the docs and the code don't match up. When that happens I explain in the commit
Blocked