Add Node::unparse and tweak the cursor creation API#666
Add Node::unparse and tweak the cursor creation API#666Xanewok merged 14 commits intoNomicFoundation:mainfrom
Node::unparse and tweak the cursor creation API#666Conversation
...than `Default::default()`, which doesn't say much at the call-site.
🦋 Changeset detectedLatest commit: 1007589 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 |
OmarTawfik
left a comment
There was a problem hiding this comment.
Left a few questions.
| return None; | ||
| } | ||
|
|
||
| pub fn as_rule_with_kind(&self, kinds: &[RuleKind]) -> Option<&Rc<RuleNode>> { |
There was a problem hiding this comment.
Possibly unrelated to this PR, but asking since it is addressing API feedback:
For as_rule_matching(...) .. I wonder how is that different from .as_rule().filter(...), given it is the same characters/effort to type?
Are we duplicating Rust std:: methods? what new scenarios does it enable?
Same question for as_token_matching().
There was a problem hiding this comment.
You're right that this is merely a convenience/"combined" method.
This API is from when, as I understand it, we wanted to have parity in the Rust/TS API. Since we decided against that recently, we can start thinking whether we want it or not on Rust side (AIUI it's more performant to keep that on TS side).
My first reaction was the same as yours, i.e as_{rule,token}_matching is probably redundant on the Rust side, now that we have introduced convenience as_{rule,token} at some point.
I do see some point in the convenience _with_kind functions, as as_{rule,token}_with_kind reads and formats better than the as_{rule,token}().filter(|node| &[some,kinds].contains(node.kind)) chain. WDYT?
There was a problem hiding this comment.
I like these semantically rich apis that match common use cases. I'm also happy to have them in a separate trait, such as RichXXXAPI or EnhancedXXX etc, simply for the sake of having a 'core' minimal API and then a layered convenience API.
There was a problem hiding this comment.
Specifically because it reads well in the code to express clearly the higher-level intent. And I like descriptive precise names, with no concern at all about the length of names.
There was a problem hiding this comment.
For as_rule_matching(...) .. I wonder how is that different from .as_rule().filter(...), given it is the same characters/effort to type?
Same to type, but less to understand.
There was a problem hiding this comment.
I do see some point in the convenience
_with_kindfunctions, asas_{rule,token}_with_kindreads and formats better than theas_{rule,token}().filter(|node| &[some,kinds].contains(node.kind))chain. WDYT?
Same to type, but less to understand.
with_kind() makes sense. The alternative doesn't have a good Rust standard library alternative, and I imagine it is a common use case, enough to have a helper for.
But on the other hand, as_rule_matching() here is longer than the Rust standard library alternative (.as_rule().filter()), which most Rust users are already familiar with, and is used extensively in all projects, so I feel we are only replacing the language built-in API with a longer/less idiomatic one, that is not really easier to use (both accept the same kind of callback).
There was a problem hiding this comment.
Agree, I'm not so sure about the _matching variant, especially since as(...).filter(...) does the same and filter is bread-and-butter idiomatic Rust.
I can understand the readability context a bit but then again, if it's not using the std impls then that might imply something non-standard, actually.
I'm fine either way, to be honest!
crates/solidity/outputs/cargo/tests/src/doc_examples/simple_contract.rs
Outdated
Show resolved
Hide resolved
Node::reconstruct and tweak the cursor creation APINode::unparse and tweak the cursor creation API
OmarTawfik
left a comment
There was a problem hiding this comment.
Left one suggestion about removing as_XXX_matching(), but otherwise, LGTM!
This is more vanilla and we didn't really use those internally ourselves, so I'm not sure if there's much benefit for those. We can reintroduce a `NodeHelpers` trait in the future that has that convenience API.
|
I think it makes sense to drop the individual |
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.12.0 ### Minor Changes - [#699](#699) [`ddfebfe9`](ddfebfe) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove `ProductionKind` in favor of `RuleKind` - [#699](#699) [`ddfebfe9`](ddfebfe) Thanks [@Xanewok](https://github.com/Xanewok)! - Allow parsing individual precedence expressions, like `ShiftExpression` - [#665](#665) [`4b5f8b46`](4b5f8b4) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove the CST Visitor API in favor of the Cursor API - [#666](#666) [`0434b68c`](0434b68) Thanks [@Xanewok](https://github.com/Xanewok)! - Add `Node::unparse()` that allows to reconstruct the source code from the CST node - [#675](#675) [`daea4b7f`](daea4b7) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename `Cursor`'s `pathRuleNodes()` to `ancestors()` in the NodeJS API. - [#676](#676) [`b496d361`](b496d36) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Fix NAPI `cursor` types and expose `cursor.depth`. ### Patch Changes - [#685](#685) [`b5fca94a`](b5fca94) Thanks [@Xanewok](https://github.com/Xanewok)! - `bytes` is now properly recognized as a reserved word - [#660](#660) [`97028991`](9702899) Thanks [@Xanewok](https://github.com/Xanewok)! - Drop List suffix from collection grammar rule names Co-authored-by: github-actions[bot] <github-actions[bot]@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.12.0 ### Minor Changes - [#699](NomicFoundation/slang#699) [`ddfebfe9`](NomicFoundation/slang@ddfebfe) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove `ProductionKind` in favor of `RuleKind` - [#699](NomicFoundation/slang#699) [`ddfebfe9`](NomicFoundation/slang@ddfebfe) Thanks [@Xanewok](https://github.com/Xanewok)! - Allow parsing individual precedence expressions, like `ShiftExpression` - [#665](NomicFoundation/slang#665) [`4b5f8b46`](NomicFoundation/slang@4b5f8b4) Thanks [@Xanewok](https://github.com/Xanewok)! - Remove the CST Visitor API in favor of the Cursor API - [#666](NomicFoundation/slang#666) [`0434b68c`](NomicFoundation/slang@0434b68) Thanks [@Xanewok](https://github.com/Xanewok)! - Add `Node::unparse()` that allows to reconstruct the source code from the CST node - [#675](NomicFoundation/slang#675) [`daea4b7f`](NomicFoundation/slang@daea4b7) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename `Cursor`'s `pathRuleNodes()` to `ancestors()` in the NodeJS API. - [#676](NomicFoundation/slang#676) [`b496d361`](NomicFoundation/slang@b496d36) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - Fix NAPI `cursor` types and expose `cursor.depth`. ### Patch Changes - [#685](NomicFoundation/slang#685) [`b5fca94a`](NomicFoundation/slang@b5fca94) Thanks [@Xanewok](https://github.com/Xanewok)! - `bytes` is now properly recognized as a reserved word - [#660](NomicFoundation/slang#660) [`97028991`](NomicFoundation/slang@9702899) Thanks [@Xanewok](https://github.com/Xanewok)! - Drop List suffix from collection grammar rule names Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Closes #583
Ref #628 for the modified cursor/offset API
Changes are best reviewed commit-by-commit.