docs: repo integration guide #3767

jeysal · 2022-11-16T17:38:27Z

Resolves #3708

Took a little more time today to almost finish this, just gotta write a little bit about package manager workspaces/lerna.

I'm trying to make the guide very approachable with little knowledge about any tooling configuration whatsoever. I give copy paste replacements that work if you have done the basic setup recommended by Prettier/ESLint/Turborepo/... and would like to replace a tool with Rome. Of course this cannot cover complex setups and configuration in detail, but if users have done that, they probably have some experience with tooling and can figure out those remaining modifications afterwards.

At the same time, I try to nudge users to do things in a future-proof way. E.g. ignore instead of include patterns so that users will automatically benefit when we become capable of formatting additional file types. And using Rome directly in the root if possible because we'll have features for multi-package repos in the future.

netlify · 2022-11-16T17:38:31Z

✅ Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`9ac4ecc`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/637f5c120d5b620008ed5a1d
😎 Deploy Preview	https://deploy-preview-3767--docs-rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

jeysal · 2022-11-16T17:41:02Z

website/src/pages/guides/integrating-into-your-repository.mdx

+{
+  "scripts": {
+-    "lint": "next lint"
+    "lint": "rome check ."


If we've shipped it in time, we should recommend --force-colors here. Otherwise the output looks shite calling through turbo lint in the root

That will be part of the next release :) #3625

Yup I saw, just not sure if that'll be out soon enough before this is merged

jeysal · 2022-11-16T17:46:03Z

website/src/pages/guides/integrating-into-your-repository.mdx

+```diff
+"scripts": {
+-  "lint": "eslint --fix ."
+  "lint": "rome check --apply .",


This lint command is unfortunately not equivalent right now. If rome finds an error that has a suggested fix, it will print "skipped 1 suggested fix" and not highlight the problem and exit with 0. This seems unintuitive to me, does anyone know if this is intended or discussed in some issue?

I think it's intentional. rome check is supposed to be the command to make sure the code is correct. --apply and --apply-suggested are volountary commands where the user decides to fix the code and how to fix it. It seems a bit redundant to show them diagnostics (at least the ones related to linting).

Okay. That still seems unintuitive to me to be honest, as flags usually only alter/augment the behavior of a command and this seems to be more like a completely different command (rome apply).
I think in this guide I can still leave it as a recommendation despite the different behavior, since I'm not recommending anyone to use this command for CI or anything dangerous like that.

It makes sense, thank you for the suggestion.

FYI we plan to remove --apply-suggested in the next months. This command was created as a quick workaround to allow users to apply also suggested fixes, but the correct way to do so would be to use a rome check --review argument that enables an interactive review where users can decide to:

apply the suggested fix

or suppress the rule

website/src/pages/guides/integrating-into-your-repository.mdx

Co-authored-by: Superchupu <53496941+SuperchupuDev@users.noreply.github.com>

jeysal · 2022-11-20T19:40:43Z

website/src/pages/guides/integrating-into-your-repository.mdx

+```json
+{
+	"formatter": {
+		"ignore": ["build", "**/*.generated.js"]


This and similar code examples should likewise also be adapted when we ship files.ignore, again not sure if release will be before this doc merge

jeysal · 2022-11-20T20:17:18Z

website/src/pages/guides/integrating-into-your-repository.mdx

+}
+```
+
+Do not quote a pattern like `**/*.ts`. Most modern package managers are able to handle it correctly on all platforms.


IIRC there's an issue to implement glob patterns in Rome. I think it would be useful to do that to reduce reliance on package managers here. Recommending to run through package manager scripts is generally a bit of an eyesore to me, because it's so much slower. It would be nice to have a better way that we can confidently recommend to people. In the meantime, any further dependency on package managers such as for globbing would be good to avoid for future-proofing

jeysal · 2022-11-20T20:31:04Z

Alright, happy with this first version. If we notice a significant amount of people struggling with a particular part, we can make optimizations / further expand there.

ematipico · 2022-11-21T10:43:02Z

website/src/pages/guides/integrating-into-your-repository.mdx

+-    "format": "prettier --write ."
+    "format": "rome format --write .",
+-    "ci:format": "prettier --check ."
+    "ci:format": "rome format .",


I don't think this is the correct transition, rome format doesn't return an error code here, while prettier does. The correct way for us to suggest the transition for CI environments should be:

rome ci --linter-enabled=false .

Ohh that's true, I never looked at the exit code, only assumed it would exit 1 if it prints the formatter diff. I will change this for the guide.
Are you aware of discussion threads or so on the CLI design? There have been a few design decisions now that struck me as odd compared to existing JS tools and also other CLI tools, would be good to get more context or to get involved if there are still active discussions about potential changes

There have been some issue around the unclearness/oddity of how error codes are returned from our commands.

rome format . does not log to stderr #3168

🐛 rome format with errors exits with code 0 #3443

There have been talks between the staff team about using error codes in a better way (a la' prettier), and we do want to use this Rust primitive, but we haven't had the time to look into it.

Feel free to start a github discussion if you have suggestions

I commented on #3443 to keep it in one place. I think there's a good way forward.

Path forward for this and the other instances of it that I have in mind: We recommend an example like

"format": "rome format --write .", "check:format": "rome format --check .", "lint": "rome check --fix .", "check:lint": "rome check ."

but add a CI section like at the start of the guide to clarify that CI should run not these but rome ci (if necessary with parts of Rome disabled via rome.json)

website/src/pages/guides/integrating-into-your-repository.mdx

jeysal · 2022-11-22T16:29:20Z

@sebmck @MichaReiser feel free to also leave a review on whether this is coherent and the suggestions made to users are good 😅

jeysal · 2022-11-23T20:50:09Z

Probably best to wait with merging this until we have resolved the awkwardness of not having good replacements for prettier --check and eslint --fix (see review comment threads), and also would be nice to have things like files.ignore and --force-colors released already.
Then all the recommendations we give people for their setup will actually be solid.
Will make the required edits and resolve the comments once #3443 and #3837 have reached a conclusion.

* upstream/main: (73 commits) fix(semantic_analyzers): style/noShoutyConstants does not recognize multiple uses of a constant. (rome#3789) feat(rome_js_analyze): useDefaultSwitchClauseLast (rome#3791) chore: run rustfmt and typo fix (rome#3840) feat(rome_js_analyze): use exhaustive deps support properties (rome#3581) website(docs): Fix text formatting (rome#3828) feat(rome_js_analyze): `noVoidTypeReturn` (rome#3806) feat(rome_cli): expose the `--verbose` flag to the CLI (rome#3812) fix(rome_diagnostics): allow diagnostic locations to be created without a resource (rome#3834) feat(rome_js_analyze): add noExtraNonNullAssertion rule (rome#3797) fix(rome_lsp): lsp friendly catch unwind (rome#3740) feat(rome_js_semantic): model improvements (rome#3825) feat(rome_json_parser): JSON Lexer (rome#3809) feat(rome_js_analyze): implement `noDistractingElements` (rome#3820) fix(rome_js_formatter): shothanded named import line break with default import (rome#3826) feat(rome_js_analyze): `noConstructorReturn` (rome#3805) feat(website): change enabledNurseryRules to All/Recommended select (rome#3810) feat(rome_js_analyze): noSetterReturn feat(rome_js_analyze): noConstructorReturn feat(rome_analyze): suppress rule via code actions (rome#3572) feat(rome_js_analyze): `noVar` (rome#3765) ...

elijaholmos · 2022-12-03T01:05:59Z

website/src/pages/guides/integrating-into-your-repository.mdx

+
+### CI
+
+If you have a CI pipeline, such as GitHub Actions, we recommend that you check formatting and linting as part of it by adding the following command:


I think it would be practical to include a guide on (or at the very least, a reference to) the Rome GitHub action & how it can be setup in a GitHub repository.

WIP docs: repo integration guide

0eceb4b

consistent pronoun writing style

81b2d94

jeysal commented Nov 16, 2022

View reviewed changes

SuperchupuDev reviewed Nov 17, 2022

View reviewed changes

website/src/pages/guides/integrating-into-your-repository.mdx Outdated Show resolved Hide resolved

Update website/src/pages/guides/integrating-into-your-repository.mdx

b785b71

Co-authored-by: Superchupu <53496941+SuperchupuDev@users.noreply.github.com>

jeysal commented Nov 20, 2022

View reviewed changes

workspaces/lerna guide section

5126da4

jeysal commented Nov 20, 2022

View reviewed changes

cleanup, consistency & fixes

183a666

jeysal changed the title ~~WIP docs: repo integration guide~~ docs: repo integration guide Nov 20, 2022

jeysal marked this pull request as ready for review November 20, 2022 20:31

jeysal requested a review from a team November 20, 2022 20:31

ematipico suggested changes Nov 21, 2022

View reviewed changes

jeysal mentioned this pull request Nov 23, 2022

🐛 rome format with errors exits with code 0 #3443

Open

1 task

jeysal added 2 commits November 24, 2022 12:53

rome ci opting out of features

9ac4ecc

elijaholmos reviewed Dec 3, 2022

View reviewed changes

jeysal closed this Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: repo integration guide #3767

docs: repo integration guide #3767

jeysal commented Nov 16, 2022

netlify bot commented Nov 16, 2022 •

edited

Loading

jeysal Nov 16, 2022

ematipico Nov 17, 2022

jeysal Nov 20, 2022

jeysal Nov 16, 2022

ematipico Nov 17, 2022 •

edited

Loading

jeysal Nov 20, 2022

ematipico Nov 21, 2022

jeysal Nov 23, 2022

jeysal Nov 20, 2022

jeysal Nov 20, 2022

jeysal commented Nov 20, 2022

ematipico Nov 21, 2022 •

edited

Loading

jeysal Nov 22, 2022

ematipico Nov 22, 2022 •

edited

Loading

ematipico Nov 22, 2022

jeysal Nov 23, 2022

jeysal Nov 24, 2022

jeysal commented Nov 22, 2022

jeysal commented Nov 23, 2022 •

edited

Loading

elijaholmos Dec 3, 2022


		### CI

		If you have a CI pipeline, such as GitHub Actions, we recommend that you check formatting and linting as part of it by adding the following command:

docs: repo integration guide #3767

docs: repo integration guide #3767

Conversation

jeysal commented Nov 16, 2022

netlify bot commented Nov 16, 2022 • edited Loading

✅ Deploy Preview for docs-rometools ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeysal commented Nov 20, 2022

ematipico Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeysal commented Nov 22, 2022

jeysal commented Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

netlify bot commented Nov 16, 2022 •

edited

Loading

ematipico Nov 17, 2022 •

edited

Loading

ematipico Nov 21, 2022 •

edited

Loading

ematipico Nov 22, 2022 •

edited

Loading

jeysal commented Nov 23, 2022 •

edited

Loading