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

fix(rome_js_formatter): abstract must precede accessor #4242

Merged
merged 2 commits into from
Mar 1, 2023
Merged

fix(rome_js_formatter): abstract must precede accessor #4242

merged 2 commits into from
Mar 1, 2023

Conversation

Conaclos
Copy link
Contributor

@Conaclos Conaclos commented Feb 26, 2023

Summary

While I was updating prettier snapshots in #4234, cargo test failed on a mis-formatted code.

The following code:

abstract class C { abstract accessor prop: number }

was formatted at the following syntactically invalid code:

abstract class C { accessor abstract prop: number }
                            ^^^^^^^^
                            'abstarct' must precede 'accessor'

This PR fixes this issue.
In the future, code could be refactored in order to use the enum that encodes modifier precedence in the parser.

Note: By the way, I am not sure to understand why the formatter tries to fix the modifier order if the parser errors as soon as a mis-ordered modifier is found.

Test Plan

I included a test for the formatter.

Documentation

No change.

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Feb 26, 2023

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 6e8c973
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/63ff3bf266527100085dc30c
😎 Deploy Preview https://deploy-preview-4242--docs-rometools.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@denbezrukov
Copy link
Contributor

By the way, I am not sure to understand why the formatter tries to fix the modifier order if the parser errors as soon as a mis-ordered modifier is found.

I think because we need to cover this case:
https://docs.rome.tools/playground/?code=YwBsAGEAcwBzACAAVABlAHMAdAAgAHsACgAgACAAIAAgAGQAZQBjAGwAYQByAGUAIABwAHIAaQB2AGEAdABlACAAcgBlAGEAZABvAG4AbAB5ACAAdABlAHMAdAA%2FADoAIABzAHQAcgBpAG4AZwA7AAoAfQA%3D

@Conaclos Conaclos changed the title fix: accessor must appear after abstract fix(rome_js_formatter): accessor must appear after abstract Feb 26, 2023
@Conaclos Conaclos changed the title fix(rome_js_formatter): accessor must appear after abstract fix(rome_js_formatter): abstract must precede accessor Feb 26, 2023
@Conaclos Conaclos merged commit 1427725 into rome:main Mar 1, 2023
@Conaclos Conaclos deleted the rome_js_syntax/fix_modifier_precedence branch March 7, 2023 16:30
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.

3 participants