Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Rename *Literal nodes to *LiteralExpression #1800

Merged
merged 3 commits into from
Nov 22, 2021

Conversation

MichaReiser
Copy link
Contributor

Summary

#1799 introduced a new JsLiteralMemberName that is defined as JsLiteralMemberName = value: ('ident' | 'js_string_literal_token' | 'js_number_literal_token'). That means, quering for all StringLiterals using root.descendants().filter(JsStringLiteral::cast) will not return the string_literal_token inside of the JsLiteralMemberName.

This is expected, because the JsLiteralMemberName contains the token and not the JsStringLiteral node but it may be unexpected for pass authors.

I believe that not using a JsStringLiteral inside of member names or import statements (module name) is desired because authors may otherwise be inclined to, for example, replace all JsStringLiteral with JsTemplateLiteralExpression but that will create an invalid syntax tree in the case of import statements.

That's why this PR renames Js*Literals to Js*LiteralExpression to make it clear that these are the literal expressions. It then renames JS_*_LITERAL_TOKEN to JS_*_LITERAL as we no longer need the token postfix to avoid the name collision.

The PR does not change the parsing for PropName since this will go away soon anyway.

Test Plan

Verified the changes rast outputs to make sure the parser changes were correctly applied.

@ematipico
Copy link
Contributor

@MichaReiser as you can see the CI is not triggering any action. I think you should add feature/member-expression branch to the CI.

I think we should come up with a naming convention for branches that will require multiple PRs and still triggering the CI (myself and yourself are doing this).

Something like multiple-feature/* and branches like this will trigger the CI too (not just main).

@MichaReiser
Copy link
Contributor Author

The CI job will run when the base branch is merged.

@ematipico
Copy link
Contributor

ematipico commented Nov 18, 2021

That's the thing. The CI is not running here and there's now way for reviewers to see if these new changes are healthy or not. What if the coverage goes down? Lint issues?

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 22, 2021

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 71a166b
Status: ✅  Deploy successful!
Preview URL: https://179552f3.tools-8rn.pages.dev

View logs

Base automatically changed from feature/member-expression to main November 22, 2021 15:19
@github-actions
Copy link

Test262 comparison coverage results on ubuntu-latest

Test result main count This PR count Difference
Total 17608 17608 0
Passed 16787 16787 0
Failed 820 820 0
Panics 1 1 0
Coverage 95.34% 95.34% 0.00%

@github-actions
Copy link

Test262 comparison coverage results on windows-latest

Test result main count This PR count Difference
Total 17608 17608 0
Passed 16787 16787 0
Failed 820 820 0
Panics 1 1 0
Coverage 95.34% 95.34% 0.00%

@MichaReiser MichaReiser merged commit eebed7d into main Nov 22, 2021
@MichaReiser MichaReiser deleted the feature/literal-expressions branch November 22, 2021 15:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants