rome / tools

Unified developer tools for JavaScript, TypeScript, and the web
https://docs.rome.tools/
MIT License
23.74k stars 659 forks source link

🐛 public/protected/private TypeScript qualifiers removed from constructor arguments automatically by Rome #1153

Closed pvarga-dni closed 4 years ago

pvarga-dni commented 4 years ago

Environment

> rome rage --summary

Environment Variables
  Object {
    COLORTERM: "truecolor"
    LANG: "en_GB.UTF-8"
    TERM: "xterm-256color"
    TERM_PROGRAM: "vscode"
  }

User Config
  unset

Rome Version
  10.0.4-beta

Node Version
  14.8.0

Platform
  darwin x64 19.6.0

Terminal Features
  Object {
    background: "unknown"
    colorDepth: 24
    columns: 156
    cursor: true
    hyperlinks: true
    isTTY: true
    unicode: true
  }

Client Flags
  Object {
    clientName: "cli"
    cwd: "/redacted"
    realCwd: "/redacted"
    silent: false
  }

Server Status
  Object {
    projects: Array [
      Object {id: 0}
      Object {id: 1}
    ]
    server: Object {
      heapTotal: 62_939_136
      pid: 16_160
      uptime: 0.54532019
    }
    workers: Array [
      Object {
        astCacheSize: 0
        heapTotal: 62_939_136
        ownedBytes: 0
        ownedFileCount: 0
        pid: 16_160
        uptime: 0.545224275
      }
    ]
  }

Steps To Reproduce

  1. Ran rome check --review but then didn't accept any of the proposed changes (pressed n).
  2. Modifications like this are made to code: class X { constructor(public y: z, -> class X { constructor (y,

Expected Results

Just fix formatting but don't remove the visibility modifiers and types.

sebmck commented 4 years ago

JSBindingIdentifier and JSBinding* formatter methods need to be updated to output meta.accessibility.

ematipico commented 4 years ago

@sebmck I started looking at it and it's not only a format issue but also the parser. That information is not present in the final AST.

sebmck commented 4 years ago

That's weird, parseBindingListItem and the parseTSAccessModifier call is where we'd be setting it.