rome / tools

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

📎 Add configuration option to remove semicolons #3535

Closed michaelhays closed 1 year ago

michaelhays commented 1 year ago

Description

Prettier has the option to remove semicolons from your code, except for in certain instances to prevent changing the meaning of the code.

It would be great to have this as a configuration option for Rome, particularly to help the adoption rate for existing codebases that don't use semicolons.

(Creating this issue as a continuation of a discussion in this project, to track interest in adding this option)

sebmck commented 1 year ago

This has been a much requested feature. @rome/staff How hard would this be to implement?

SuperchupuDev commented 1 year ago

would this also add the ability to add semicolons?

sebmck commented 1 year ago

@SuperchupuDev I'm not sure I know what that means? This issue is for a configuration option that causes semicolons to be omitted when they aren't necessary. Similar to the prettier option.

SuperchupuDev commented 1 year ago

nevermind, i was talking about something like the semi option from prettier to add instead of removing semicolons even if they aren't necessary, but looks like rome already does the same by default. maybe the configuration option described by this issue could be a semicolon option (that can be set to false) instead of a no-semicolon option? sounds more intuitive

MichaReiser commented 1 year ago

This has been a much requested feature. @rome/staff How hard would this be to implement?

I don't expect it to be too complicated. It's probably a day or two of work.

This is prettier's logic that we can us as a starting point.

https://github.com/prettier/prettier/blob/199a1bb38929c72861ead3cf44978a895eba65bb/src/language-js/print/statement.js#L89-L157

Let's see how today goes. Maybe I'll find some time to look into it.

ysageev commented 1 year ago

I love the idea of Rome but cannot use the VS Code extension because semicolons. ☚ī¸

michaelhays commented 1 year ago

They clearly understand the need for this, please be patient!

Just leave a :+1: on this issue to express your support.

sebmck commented 1 year ago

Draft PR now open from @MichaReiser in #3702 đŸ¤¯đŸĨŗ

MichaReiser commented 1 year ago

Could I ask you to vote for the preferred positioning of semicolons in the cases where the semicolon is required? It would help me to decide on the formatting that Rome should implement.

alistairholt commented 1 year ago

Just wanted to say that it's been great to see how promptly and professionaly this was actioned ❤ī¸

MichaReiser commented 1 year ago

Just wanted to say that it's been great to see how promptly and professionaly this was actioned ❤ī¸

Thank you! I plan to push a pre-release tomorrow and would appreciate it if you could try it and report any issues related to the new semicolons options. I'll post some more detailed instructions tomorrow

MichaReiser commented 1 year ago

I've published a nightly Rome release (extension and CLI) a few minutes ago. Could you try the new version and report any semicolon-related issues (feel free to tag me)?

npm i rome@nightly
npx rome format --semicolons=as-needed <paths>

or create a rome configuration

npx rome init 

and set the option in the rome.json file

{
  "javascript": {
    "formatter": {
      "semicolons": "asNeeded"
    }
  }
}

and then run npx rome format <paths>

Thanks!

ysageev commented 1 year ago

Pre-release extension is super fast and no semicolons in sight. đŸ•ē

With the CLI (which I don't really use), I get:

image

I'm not sure if I'm reading it correctly, but looks like it wants to put semicolons in.

LinusU commented 1 year ago

@ysageev how are you configuring/running rome? It seems like it also wants to change spaces to tabs, so it might not be picking up your config...

ysageev commented 1 year ago

My rome.js. I know it is picking up the config because the linter does not flag the disabled rules.

   "linter": {
      "enabled": true,
      "rules": {
         "recommended": true,
         "security": {
            "noDangerouslySetInnerHtml": "off"
         },
         "a11y": {
            "useKeyWithClickEvents": "off"
         },
         "correctness": {
            "noArguments": "off"
         }
      }
   },
   "javascript": {
      "formatter": {
         "semicolons": "asNeeded"
      }
   }
}
MichaReiser commented 1 year ago

Thanks for giving it a try!

Pre-release extension is super fast and no semicolons in sight. đŸ•ē

With the CLI (which I don't really use), I get:

image

Why are there elephants at the end of every line? 😆

I'm not sure if I'm reading it correctly, but looks like it wants to put semicolons in.

Seems like. What's the command you run (ci?), and do you run it in the root folder of your project (the folder that contains the rome.json)?

ysageev commented 1 year ago

Seems like. What's the command you run (ci?), and do you run it in the root folder of your project (the folder that contains the rome.json)?

Aha! You are right. I didn't want to format all of my files in ./src, so I cd'd into /src/components/data and ran the file on Aggregate.js.

When instead I now ran npx rome format ./src/components/data/Aggregate.js I got the correct result:

image

Why are there elephants at the end of every line? 😆

I'm running on Git bash on Windows 10. I thought they were weird percent symbols that signal some special EOL to the formatter. But, on zooming in, they are indeed elephants, which are way better.

Definitely keep the elephants.

LinusU commented 1 year ago

@MichaReiser it seems to work great! However, our code base didn't currently use any "necessary" semicolons, so I cannot comment on that part.

Our code base isn't currently using Standard, but I'm interested in switching to Rome. In the end, it was too many changes to switch right now (it's a big project, and I don't want to mess up the git blame too much). But I'll be following along enthusiastically, and when the linter is ready to replace ESLint for us I'll make the switch to the format tool as well!

ematipico commented 1 year ago

@MichaReiser it seems to work great! However, our code base didn't currently use any "necessary" semicolons, so I cannot comment on that part.

Our code base isn't currently using Standard, but I'm interested in switching to Rome. In the end, it was too many changes to switch right now (it's a big project, and I don't want to mess up the git blame too much). But I'll be following along enthusiastically, and when the linter is ready to replace ESLint for us I'll make the switch to the format tool as well!

To not mess up git blame, you can use these kinds of files: https://github.com/rome/tools/blob/main/.git-blame-ignore-revs

They are really useful. We also contracted these kinds of huge changes (switch from tabs to spaces). I hope it helps!

michaelhays commented 1 year ago

@MichaReiser I just tested the nightly build on my existing codebase of ~50,000 LOC, and it works perfectly! No changes whatsoever in semicolons from the prettier output.

Really impressive work :)