chore: add prettier by theVedanta · Pull Request #949 · uwdata/mosaic

theVedanta · 2025-12-24T23:38:30Z

No description provided.

domoritz

At the moment, we don't use prettier.

To add this we would need to

make prettier a dev dependency
add prettier support to eslint
ignore schema files

We should make sure that there are no degredations after we add prettier.

domoritz · 2026-01-07T02:24:06Z

.prettierrc.json

+{
+  "singleQuote": true,
+  "tabWidth": 2,
+  "trailingComma": "none"


I think we want a print width of 120

theVedanta · 2026-01-07T02:51:09Z

At the moment, we don't use prettier.

To add this we would need to

make prettier a dev dependency

add prettier support to eslint

ignore schema files

We should make sure that there are no degredations after we add prettier.

Addressed these. Hope I added all the necessary schema files in prettierignore

Also added printWidth

domoritz · 2026-01-07T03:01:54Z

Do we need a prettierignore? Doesn't eslint do that? Or is that for editors with auto formatter?

theVedanta · 2026-01-07T03:08:12Z

I think we might need prettierignore for running it in the terminal, or for the editor's format_on_save, I couldn't find an eslint config option for the same. Sorry if I'm missing something (?)

domoritz · 2026-01-07T03:16:32Z

Makes sense. Eslint will also support format on save.

domoritz · 2026-01-07T03:17:12Z

Ci is failing btw. We probably need to reformat as well.

theVedanta · 2026-01-07T03:18:23Z

yep, lemme check it out

eslint.config.js

derekperkins · 2026-01-07T23:38:59Z

Should we add a command to package.json to write the changes?

domoritz · 2026-01-08T15:29:40Z

Yes

theVedanta · 2026-01-08T15:49:29Z

I added the script and formatted the files in two separate commits; I don't know if the change is too drastic to review, if it is we can undo this format command and leave it up to the developers to use later (?)

domoritz · 2026-01-08T16:41:51Z

package.json

    "clean": "lerna run clean",
    "lint": "lerna run lint",
    "test": "tsc --build && lerna run schema && vitest run",
+    "write": "prettier --write '**/*.{js,ts,json,yaml}'",


No, please add format or lint:fix and let ESLint fix things.

theVedanta · 2026-01-09T21:51:08Z

I added both format and lint:fix; not sure if we want individual package scripts for this in the packages and let lerna execute

domoritz

Overall looks reasonable with a few comments below. Thanks for your help here!

domoritz · 2026-01-09T22:13:14Z

docs/public/contours.yaml

 params:
  bandwidth: 50
  thresholds: 10
 plot:


Isn't this generated? Then we should not reformat.

what other files would this go for? just making sure I know what else is generated here

I think the yaml and esm example are all generated. Try building the library and website without formatting and see whether anything gets reset.

domoritz · 2026-01-09T22:13:34Z

package.json

    "test": "tsc --build && lerna run schema && vitest run",
+    "format": "prettier --write '**/*.{js,ts,tsx,json,css,md}'",
+    "lint:fix": "eslint '**/*.{js,ts,tsx}' --fix",
+    "format:check": "prettier --check '**/*.{js,ts,tsx,json,css,md}'",


That should be in lint already.

what should be in lint? Sorry I don't understand?

Lint should be a superset of format check

gotcha, removing.

domoritz · 2026-01-09T22:13:49Z

package.json

    "clean": "lerna run clean",
    "lint": "lerna run lint",
    "test": "tsc --build && lerna run schema && vitest run",
+    "format": "prettier --write '**/*.{js,ts,tsx,json,css,md}'",


Lint fix and format should be the same

would having prettier-format not address more of files that eslint cannot go over though?

Maybe. But then we should still have them in one command. I am also less worried about files other than ts.

That makes sense. would you like me to make a combined command for them or just get rid of the prettier --write entirely?

How much do we benefit from combined vs remove?

How about we do:

"lint:fix": "eslint '**/*.{js,ts,tsx}' --fix && prettier --write '**/*.{js,ts,tsx,json,css,md}'",

and then have a format:write command separately for the --write? Best of both worlds imo

I don't suppose it's any harm removing the format command though; if we dont care about the files other than ts

domoritz · 2026-01-09T22:14:31Z

packages/examples/vanilla-example/src/main.js

+document
+  .getElementById('plot')
+  .replaceChildren(
+    vg.plot(vg.lineY(vg.from('aapl'), { x: 'Date', y: 'Close', tip: true }), vg.width(680), vg.height(200))


This is worse in terms of readability. Any ideas how we can improve?

umm.. I would say

printWidth should be shorter like 80 or so (I tested 80 and works okay for me)

es5 for the trailing commas

Doing that now...

Let's try different line widths and see what gives us good results and fewer changes.

Why would trailing commas help?

Maybe 100 is good

Yeah 100 works well for me too :)
We dont wanna use "all" for it since trailing commas are not seen in the project, so to keep things close to the previous version and reduce changes, I thought "es5" would be a good idea

domoritz · 2026-01-09T22:15:15Z

packages/examples/vega-example/src/main.js

+  .querySelector('#plots')
+  .replaceChildren(
+    vg.vconcat(
+      vg.hconcat(vg.hspace('2em'), vg.menu({ from: 'weather', column: 'weather', label: 'Weather', as: selection })),


This also looks worse. But maybe okay as a trade off.

packages/mosaic/core/src/Coordinator.ts

domoritz · 2026-01-09T22:16:50Z

packages/mosaic/core/src/MosaicClient.ts

   * @returns this
   */
-  queryError(error: Error): this { // eslint-disable-line @typescript-eslint/no-unused-vars
+  queryError(error: Error): this {


Why is this in a new line?

good catch! This is interesting because the printwidth will wrap it to the next line. We have the option to:

Move all these comments above the functions manually with eslint-disable-next-line (time-taking)

Rename all unused args to start with _ perhaps? and then ignore them in global eslint config with something like "@typescript-eslint/no-unused-vars": ["error", { "argsIgnorePattern": "^_" }]

1 seems cleaner. Even better might be to disable the check and use typescript with @ts-expect-error. The advantage of that is that this will error when the variable is actually used.

Pardon me, but why would you say that is an advantage?

ts expect error will silence an error but also will error if there is no error

domoritz · 2026-01-09T22:17:36Z

packages/mosaic/core/src/QueryConsolidator.ts

  const sql = `${query}`;
  if (isSelectQuery(query) && !cache.get(sql)) {
    if (
-      query._where.length || query._qualify.length || query._having.length ||


Was there a reason for the grouping? Maybe use comments.

domoritz · 2026-01-09T22:19:45Z

packages/mosaic/core/src/preagg/sufficient-statistics.ts

-      sub(y, avg(y as ColumnRefNode))
-    )
-  );
+  const sxy = sum(mul(sub(x, avg(x as ColumnRefNode)), sub(y, avg(y as ColumnRefNode))));


Harder to read

…use /flat & arrowParens

domoritz · 2026-01-13T16:44:24Z

.prettierrc.json

  "singleQuote": true,
  "tabWidth": 2,
-  "trailingComma": "none",
-  "printWidth": 120,


I think I prefer 100.

Gotcha, will set it to 100

…es + remove format:check

domoritz

Thank you. Just a few comments left and then we can merge. I don't see many code changes which is surprising but also good.

domoritz · 2026-02-04T14:40:57Z

bin/publish-schema.js

  try {
    await unlink(linkFile);
-  } catch (err) { // eslint-disable-line @typescript-eslint/no-unused-vars
+    // eslint-disable-next-line


I don't think this is needed anymore.

Update the rest as well. We should probably have something like

export default tseslint.config({ rules: { // Note: you must disable the base rule as it can report incorrect errors "no-unused-vars": "off", "@typescript-eslint/no-unused-vars": "error" } });

domoritz · 2026-02-04T14:42:58Z

packages/mosaic/core/src/util/AsyncDispatch.ts

-    _type: string, // eslint-disable-line @typescript-eslint/no-unused-vars
-    _value: unknown // eslint-disable-line @typescript-eslint/no-unused-vars
+    // eslint-disable-next-line
+    _type: string, // @typescript-eslint/no-unused-vars


We can handle _ automatically. Please read https://typescript-eslint.io/rules/no-unused-vars/

domoritz · 2026-02-04T14:44:08Z

packages/vgplot/plot/src/interactors/util/get-datum.js

    el = el.children[0];
  }
-  // @ts-ignore
+  // @ts-expect-error


Let's include a description of what error is expected here.

domoritz · 2026-02-04T14:44:17Z

packages/vgplot/plot/src/interactors/util/get-field.js

  if (isNode(field)) {
    if (field.type === 'COLUMN_REF') {
-      // @ts-ignore
+      // @ts-expect-error


Same for this and all others.

domoritz · 2026-02-04T14:44:26Z

packages/vgplot/plot/src/marks/util/extent.js


  [filter].flat().forEach(p => walk(p, (node) => {
    if (node instanceof BetweenOpNode && `${node.expr}` === column) {
-      // @ts-ignore


domoritz · 2026-02-04T14:44:53Z

packages/vgplot/spec/src/ast/ASTNode.js

-  codegen(ctx) { // eslint-disable-line no-unused-vars
-    // @ts-ignore
+   // eslint-disable-next-line
+   codegen(ctx) { // no-unused-vars


Are we not removing no-unused-vars?

domoritz · 2026-02-04T14:46:58Z

packages/vgplot/spec/src/ast/ASTNode.js

-  instantiate(ctx) { // eslint-disable-line no-unused-vars
-    // @ts-ignore
+   // eslint-disable-next-line
+   instantiate(ctx) { // no-unused-vars


Are we not removing no-unused-vars?

package.json

theVedanta · 2026-02-07T20:39:25Z

Thank you. Just a few comments left and then we can merge. I don't see many code changes which is surprising but also good.

I haven't run npm format yet, I'll do that once this commit & config look good enough. There will be some code changes then

domoritz · 2026-02-07T21:28:44Z

Oh okay. I want to see the changes before I approve.

I couldn't run the code locally since the installation was broken.

I can check locally when the installation works.

prettier

3be879a

theVedanta marked this pull request as ready for review December 24, 2025 23:38

theVedanta requested review from domoritz and jheer as code owners December 24, 2025 23:38

domoritz requested changes Jan 7, 2026

View reviewed changes

.prettierrc.json Outdated

{

"singleQuote": true,

"tabWidth": 2,

"trailingComma": "none"

Copy link

Member

domoritz Jan 7, 2026

Choose a reason for hiding this comment

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

I think we want a print width of 120

eslint + dev dep + printwidth + ignore schema-files

11c5578

domoritz changed the title ~~Prettierrc added~~ chore: add pettier Jan 7, 2026

theVedanta added 3 commits January 6, 2026 21:24

test ci

0f1bc65

lock

3880433

Merge branch 'main' into prettierrc

c6ee681

domoritz reviewed Jan 7, 2026

View reviewed changes

eslint.config.js Outdated Show resolved Hide resolved

eslint-flat

6c8e7eb

deps

3018b44

write script

c5e4f42

theVedanta requested a review from derekperkins as a code owner January 8, 2026 15:45

domoritz requested changes Jan 8, 2026

View reviewed changes

domoritz reviewed Jan 9, 2026

View reviewed changes

theVedanta added 2 commits January 9, 2026 17:19

format, lint:fix, format:check & Bumped up eslint-config-prettier to …

680a8db

…use /flat & arrowParens

printwidth improve + es5 trailing comma

f96c15d

theVedanta force-pushed the prettierrc branch from de5e797 to f96c15d Compare January 9, 2026 23:24

lock

983717a

domoritz reviewed Jan 13, 2026

View reviewed changes

theVedanta added 2 commits February 1, 2026 11:59

@ts-ignore switched out; eslint-disable-next-line for overflowing lin…

be04196

…es + remove format:check

merge

d6800b0

derekperkins changed the title ~~chore: add pettier~~ chore: add prettier Feb 1, 2026

domoritz reviewed Feb 4, 2026

View reviewed changes

Uh oh!

Conversation

theVedanta commented Dec 24, 2025

Uh oh!

domoritz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theVedanta commented Jan 7, 2026

Uh oh!

domoritz commented Jan 7, 2026

Uh oh!

theVedanta commented Jan 7, 2026

Uh oh!

domoritz commented Jan 7, 2026

Uh oh!

domoritz commented Jan 7, 2026

Uh oh!

theVedanta commented Jan 7, 2026

Uh oh!

Uh oh!

derekperkins commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domoritz commented Jan 8, 2026

Uh oh!

theVedanta commented Jan 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theVedanta commented Jan 9, 2026

Uh oh!

domoritz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theVedanta Jan 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

derekperkins commented Jan 7, 2026 •

edited

Loading

theVedanta Jan 11, 2026 •

edited

Loading