Fix TS api types and add some cursor tests #623
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 |
| 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 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/[email protected] ### 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 <[email protected]>
No description provided.