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

refactor(rome_syntax): improve inner_string_text #4731

Merged
merged 1 commit into from
Jul 28, 2023
Merged

refactor(rome_syntax): improve inner_string_text #4731

merged 1 commit into from
Jul 28, 2023

Conversation

Conaclos
Copy link
Contributor

@Conaclos Conaclos commented Jul 27, 2023

Summary

This PR improves the concistency of the code-base. It introduces several changes:

  • Remove QuotedString and StaticStringValue in favor of TokenText and StaticValue.
    This removes a bug in the implementation of QuotedString that was not reported yet.
  • Remove unused functions related to these types
  • Simplify StaticValue by replacing TemplateChunk with String and EmptyString
    • I removed dead (and misleading) code (comparison against -0 and +0 has no sense because we only handle positive numbers...)
    • I removed recognition of template string such as "${1}" because this leads to erroneous string values (for instance ${1n} is converted to the string 1n instead of the string 1).

There is more refactoring to do (maybe removing StaticValue in favor of an union of AST nodes). However, there are already many changes. This seems enough for this PIR.

Test Plan

Updated and new doc-tests.

@netlify
Copy link

netlify bot commented Jul 27, 2023

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 5884f3b
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64c3a50c6edef7000823735f

@github-actions github-actions bot added A-Linter Area: linter A-Formatter Area: formatter L-JavaScript Langauge: JavaScript A-Parser Area: parser L-JSON Language: JSON labels Jul 27, 2023
@Conaclos Conaclos requested a review from ematipico July 27, 2023 14:33
@ematipico ematipico changed the title refactor(rome_syntax): imrpove inner_string_text refactor(rome_syntax): improve inner_string_text Jul 27, 2023
@ematipico ematipico changed the title refactor(rome_syntax): improve inner_string_text refactor(rome_syntax): improve inner_string_text Jul 27, 2023
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best code is less code!

Amazing refactors and cleaner code. You're amazing 🚀

I left some questions and suggestions but overall we can merge it.

crates/rome_js_syntax/src/static_value.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/static_value.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/static_value.rs Show resolved Hide resolved
@Conaclos
Copy link
Contributor Author

The best code is less code!

As Antoine de Saint-Exupéry said:

La perfection est atteinte, non pas lorsqu'il n'y a plus rien à ajouter, mais lorsqu'il n'y a plus rien à retirer.

Meaning: "Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away."

@Conaclos Conaclos merged commit c197360 into rome:main Jul 28, 2023
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter A-Linter Area: linter A-Parser Area: parser L-JavaScript Langauge: JavaScript L-JSON Language: JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants