Fix TS api types and add some cursor tests#623
Fix TS api types and add some cursor tests#623Xanewok merged 3 commits intoNomicFoundation:mainfrom AntonyBlakey:AntonyBlakey/ts-node-access
Conversation
🦋 Changeset detectedLatest commit: 37d03ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Xanewok
left a comment
There was a problem hiding this comment.
Thanks for correcting the API types and the tests!
It'd be great to enforce types being valid as I also ran in to this the other time.
| console.log("cursor find ratio", ratio); | ||
|
|
||
| // I expected this to be a lot better than 8x. | ||
| expect(ratio).toBeGreaterThanOrEqual(8); |
There was a problem hiding this comment.
I don't get a consistent >=8, the values are in the 7.4-9.5 range for me (hitting lower threshold when not warm). I'd like to bring it lower into a safe spot not to risk intermittent CI or local test failures
There was a problem hiding this comment.
I brought it down to x5 since I'd like to merge it - it should still test for the order-of-magnitude improvements while making sure it doesn't error out in the test suite. Hope you don't mind!
| expect(ratio).toBeLessThan(4); | ||
| }); | ||
|
|
||
| test("cursor find api is faster than testing the every node", () => { |
There was a problem hiding this comment.
this test is still flakey unfortunately :(
If you add any wrong/failed assertion in any other test, it will take a few milliseconds longer to render the error, slowing down the first one as well, and thus both will fail 😞
I think testing timing this way in JS is not really consistent. Not sure I understand what this test is maintaining/asserting. I wonder if there is a different way to achieve the same goal?
| const { parseTree } = language.parse(ProductionKind.SourceUnit, source); | ||
| const cursor: Cursor = parseTree.cursor; | ||
|
|
||
| expect(cursor.node.type).toBe(NodeType.Rule); |
There was a problem hiding this comment.
nit, I suggest creating an array of the expectations and iterate on them instead. IMO, It is much harder to understand and/or update the contents otherwise.
| get textLength(): text_index.TextIndex; | ||
| get text(): string; | ||
| get cursor(): Cursor; | ||
| get cursor(): cursor.Cursor; |
There was a problem hiding this comment.
I suggest making APIs like .cursor and similar ones functions instead of getters. It creates a new cursor every time, rather than returning some data (property) of the object (node) that the property belongs to. It also hints that the cursor returned should be persisted/reused, and can mutate and change independently of the node.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @nomicfoundation/slang@0.11.0 ### Minor Changes - [#625](#625) [`7bb650b`](7bb650b) Thanks [@Xanewok](https://github.com/Xanewok)! - The CST Cursor now implements the Iterator trait as part of the Rust API - [#647](#647) [`b1dced3`](b1dced3) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Require specifying an initial offset when creating a CST cursor. ### Patch Changes - [#648](#648) [`2327bf5`](2327bf5) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Support Solidity v0.8.22. - [#623](#623) [`80114a8`](80114a8) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Correct the types in the TS api by adding the correct namespaces to type references Co-authored-by: OmarTawfik <15987992+OmarTawfik@users.noreply.github.com>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @nomicfoundation/slang@0.11.0 ### Minor Changes - [#625](NomicFoundation/slang#625) [`7bb650b`](NomicFoundation/slang@7bb650b) Thanks [@Xanewok](https://github.com/Xanewok)! - The CST Cursor now implements the Iterator trait as part of the Rust API - [#647](NomicFoundation/slang#647) [`b1dced3`](NomicFoundation/slang@b1dced3) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Require specifying an initial offset when creating a CST cursor. ### Patch Changes - [#648](NomicFoundation/slang#648) [`2327bf5`](NomicFoundation/slang@2327bf5) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Support Solidity v0.8.22. - [#623](NomicFoundation/slang#623) [`80114a8`](NomicFoundation/slang@80114a8) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Correct the types in the TS api by adding the correct namespaces to type references Co-authored-by: OmarTawfik <15987992+OmarTawfik@users.noreply.github.com>
No description provided.