-
Notifications
You must be signed in to change notification settings - Fork 653
feat(rome_js_semantic): Improvements to scope resolution #3084
Conversation
Deploying with
|
Latest commit: |
bca3277
|
Status: | ✅ Deploy successful! |
Preview URL: | https://7ead17f1.tools-8rn.pages.dev |
Branch Preview URL: | https://feature-semantic-event-impro.tools-8rn.pages.dev |
2da478c
to
67b370f
Compare
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
🎉 Fixed (1244):
ts/babel
ts/microsoft
|
49271e1
to
b78e1bd
Compare
b7677fb
to
5305bcf
Compare
5305bcf
to
a768331
Compare
Out of curiosity, can you share some example? |
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's a
a.txt
file to remove. - there's a
scopes.txt
file to remove - we should use
pnpm
as package manager, unless there are exceptions (I suppose here we can safely usepnpm
)
| TS_INTERFACE_DECLARATION | ||
| TS_ENUM_DECLARATION | ||
| TS_TYPE_ALIAS_DECLARATION => { |
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.
Do we really create scopes for TS only syntax?
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.
Yes. Any reason you think we should not?
xtask/coverage/src/symbols/scope.js
Outdated
@@ -0,0 +1,40 @@ | |||
const parser = require("@babel/parser"); |
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.
Should this package be part of the PR? If so, could you please update the description of the PR and explain what's this used for?
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.
Should this package be part of the PR?
No. I will move these to another place. And push them in another PR.
a768331
to
bca3277
Compare
Argh.. Sorry, I thought I had removed these. |
Summary
This PR start to use the semantic model with our coverage test suites. See #3038.
This new "example" actually compares our scope resolution to other tools. Our compliance percentage is still quite low, especially because:
1 - Javascript tools use start/end as characters, we as bytes; when tests cases have Unicode it is very hard to compare values;
2 - Some tools treat trivia differently than us, so we need to sometimes use
text_range
, and sometimes use the trimmed range;3 - Some tools create scopes that, as I understand, are not necessary.
Test Plan