feat: Ruff Configuration Settings #2215

mattrunyon · 2024-09-09T21:52:08Z

I need to test this doesn't crash if using groovy. Not sure if the ruff version is available without initializing ruff first or what happens if it's unavailable.

Added global setting for minimap, format on save, formatter, and formatting settings
Added format on save option and format button to notebook overflow menu
Format on save triggers on both shortcut and pressing the save button
Config editor has JSON schema included
Config editor cannot be closed by clicking outside the modal, but can be closed w/ escape key

Something we should add maybe as part of this PR or a final follow-up to this feature branch is disabling certain/all linting/formatting for the console. One way we might do this is by registering the console w/ a specific URI or URI scheme so we can check it when checking if we should lint. Or include it as a prop to the MonacoProviders component. This would require some changes to the current setup of setting/using the config on MonacoProviders static members/methods

(This might not actually be breaking, it's more just notes for implementing in DHE)
BREAKING CHANGE:
The app should call MonacoUtils.init with a getWorker function that uses the JSON worker in addition to the general fallback worker when adding support for configuring ruff.

mofojed

Yes we should disable the errors in the ConsoleInput, that's not a good time. It's always giving errors for variables that do exist.
Also we need to fix up the unit tests, I think a transformIgnore in jest needed.

mofojed · 2024-09-10T17:05:54Z

packages/app-utils/src/storage/LocalWorkspaceStorage.ts

@@ -108,13 +113,13 @@ export class LocalWorkspaceStorage implements WorkspaceStorage {
        serverConfigValues,
        'showExtraGroupColumn'
      ),
-      defaultNotebookSettings:
+      notebookSettings:
        serverConfigValues?.get('isMinimapEnabled') !== undefined


Should have server definitions for the other defaults here as well, can create a ticket to document on the server in the dh-default.prop file.

How should we handle these? Should I flatten the config for notebooks instead in redux? Right now the code that merges w/ server notebookSettings would actually only add isMinimapEnabled and not use any of the other defaults. It doesn't deep merge. Could also change to a deep merge

packages/app-utils/src/storage/LocalWorkspaceStorage.ts

mofojed · 2024-09-10T17:16:55Z

packages/react-hooks/src/useDebouncedCallback.ts

 ): DebouncedFunc<(...args: TArgs) => TResult> {
+  const { leading = false, trailing = true, maxWait } = options;


It's kind of dumb the defaults are needed here, but yes this is needed.
The docs show it should default a value: https://lodash.com/docs/4.17.15#debounce
But the code doesn't actually check if the value is undefined before using it: https://github.com/lodash/lodash/blob/6a2cc1dfcf7634fea70d1bc5bd22db453df67b42/src/debounce.ts#L88

Yep I figured that out through trial and error, but good to know the code confirmed my suspicions that they were coercing falsey value to false. I should probably add tests for this

I also made this not require a stable object into the hook because to me it's better to hide that in the hook implementation since it's simple. That way the calls are more clear with useDebouncedCallback(fn, timeout, { leading: true })

packages/code-studio/vite.config.ts

mofojed · 2024-09-10T17:28:40Z

packages/components/scss/BaseStyleSheet.scss

@@ -146,7 +146,7 @@ button:focus {
 }

 span.btn-disabled-wrapper {
-  display: contents;
+  display: inline-block;


You sure this is correct? This is the reverting a previous change you made: 96641fb#diff-5297df85e91eab4d0994f382be57fbae24870c628aa9472306cc3073a0310b0a

Looks like from my past conversation w/ Don this was an attempted fix for disabled buttons w/ tooltips in flex containers. But now this causes a shift/slight shrink on the button in the modal context.

I'll try to see if there's a better fix. Looks like it was a fix for the left/right arrows when you have a lot of dashboards open so that probably is aligned incorrectly now

I'm keeping this because it seems display: contents is making the tooltips not show up at all when disabled. There's been some changes to the button component around themeing which seems to have fixed the disabled icon buttons with tooltips when I leave this as inline-block

mofojed · 2024-09-10T17:30:16Z

packages/components/src/modal/Modal.tsx

+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [isOpen]


Might need a comment why onOpened is not being included as a dep here. That seems like a bug?
Side note, Modal should likely be deprecated in favour of Dialog and DialogTrigger.

Returned to its original version.

Currently staying with Modal because Dialog uses a different background color which makes monaco's background look jarring to me

packages/console/src/monaco/RuffSettingsModal.scss

packages/console/src/monaco/RuffSettingsModal.tsx

mofojed · 2024-09-10T18:01:45Z

packages/console/src/monaco/RuffSettingsModal.tsx

+      // Register the ruff schema so users get validation and completion
+      monaco.languages.json.jsonDefaults.setDiagnosticsOptions({
+        validate: true,
+        enableSchemaRequest: true,
+        schemas: [
+          {
+            uri: 'json://ruff-schema',
+            fileMatch: [model.uri.toString()],
+            schema: ruffSchema,
+          },
+        ],
+      });


Seems like a sloppy way to do this - it's setting it on a static context whenever we open the editor, and replacing previous settings with one that just matches the new model.
Could you use monaco.editor.createModel to create a model ahead of time and re-use that? And just register the schema once?

We could augment Editor to accept a URI or even a full model. If you try to create a model w/ an existing Uri you get an error, but you can also get an existing model by Uri. In this case we could have a consistent model uri and just file match the 1 uri for ruff settings

codecov · 2024-09-13T20:38:17Z

Codecov Report

Attention: Patch coverage is 12.94118% with 148 lines in your changes missing coverage. Please review.

Project coverage is 46.54%. Comparing base (aaf4325) to head (716d351).

Files with missing lines	Patch %	Lines
packages/console/src/monaco/RuffSettingsModal.tsx	4.44%	43 Missing ⚠️
.../code-studio/src/settings/EditorSectionContent.tsx	0.00%	33 Missing ⚠️
packages/console/src/monaco/MonacoProviders.tsx	6.25%	30 Missing ⚠️
...ashboard-core-plugins/src/panels/NotebookPanel.tsx	0.00%	26 Missing ⚠️
...ges/app-utils/src/storage/LocalWorkspaceStorage.ts	0.00%	9 Missing ⚠️
packages/code-studio/src/AppRoot.tsx	0.00%	5 Missing ⚠️
packages/console/src/monaco/MonacoUtils.ts	60.00%	2 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##           feature/python-formatter    #2215      +/-   ##
============================================================
+ Coverage                     46.04%   46.54%   +0.49%     
============================================================
  Files                           695      698       +3     
  Lines                         39267    38865     -402     
  Branches                       9875     9718     -157     
============================================================
+ Hits                          18082    18088       +6     
+ Misses                        21085    20766     -319     
+ Partials                        100       11      -89

Flag	Coverage Δ
unit	`46.54% <12.94%> (+0.49%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mofojed · 2024-09-17T18:27:47Z

packages/redux/src/store.ts

    isMinimapEnabled?: boolean;
+    formatOnSave?: boolean;
+    ruffSettings?: {


Nest this under python namespace, e.g.

notebookSettings: { ... python: { linter: { isEnabled?: boolean; config: ..., }; } }

Defined on the server, namespaced under web.user or web.client (which they aren't right now), and we should automatically load all of those set values (web.client.*) and transfer them (rather than having to explicitly client.configuration.list):

web.client.notebookSettings.isMinimapEnabled web.client.notebookSettings.python.linter.isEnabled

Then do a deep merge

Added these under web.user and tested locally by adding web.user.notebookSettings.python.linter.isEnabled to my dh-defaults.prop#client.configuration.list. Then set the prop in my local prop file and tested in incognito that the server setting default was applied

mofojed

Great work!

mattrunyon added 8 commits September 9, 2024 16:35

Replace local built wasm with npm package

cf822ec

Change defaultNotebookSettings to notebookSettings

79ac809

Add format on save local to panel

8225f7f

WIP

718becf

Add schema locally, version, and finish settings modal

0be0e3c

Remove warning

0aa4768

Remove unneeded build flags

9049430

Removing console log

283e631

mattrunyon requested a review from mofojed September 9, 2024 21:52

mattrunyon self-assigned this Sep 9, 2024

Fix package lock and ruff schema version

742e551

mofojed requested changes Sep 10, 2024

View reviewed changes

mattrunyon added 2 commits September 12, 2024 18:21

Address review comments

b9e4644

Address rest of review comments

34e8d14

mattrunyon requested a review from mofojed September 13, 2024 20:33

mofojed reviewed Sep 17, 2024

View reviewed changes

Add server config values

756ad0b

mattrunyon requested a review from mofojed September 19, 2024 20:01

Fix linter errors

716d351

mofojed approved these changes Sep 20, 2024

View reviewed changes

mattrunyon merged commit 3144edb into deephaven:feature/python-formatter Sep 20, 2024
12 checks passed

mattrunyon deleted the ruff-config branch September 20, 2024 18:14

github-actions bot locked and limited conversation to collaborators Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Ruff Configuration Settings #2215

feat: Ruff Configuration Settings #2215

mattrunyon commented Sep 9, 2024 •

edited

Loading

mofojed left a comment •

edited

Loading

mofojed Sep 10, 2024

mattrunyon Sep 12, 2024

mofojed Sep 10, 2024

mattrunyon Sep 10, 2024

mofojed Sep 10, 2024

mattrunyon Sep 10, 2024

mattrunyon Sep 13, 2024

mofojed Sep 10, 2024

mattrunyon Sep 13, 2024

mofojed Sep 10, 2024

mattrunyon Sep 10, 2024

codecov bot commented Sep 13, 2024 •

edited

Loading

mofojed Sep 17, 2024 •

edited

Loading

mattrunyon Sep 19, 2024

mofojed left a comment

		): DebouncedFunc<(...args: TArgs) => TResult> {
		const { leading = false, trailing = true, maxWait } = options;

		// eslint-disable-next-line react-hooks/exhaustive-deps
		[isOpen]

feat: Ruff Configuration Settings #2215

feat: Ruff Configuration Settings #2215

Conversation

mattrunyon commented Sep 9, 2024 • edited Loading

mofojed left a comment • 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

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

codecov bot commented Sep 13, 2024 • edited Loading

Codecov Report

mofojed Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mofojed left a comment

Choose a reason for hiding this comment

mattrunyon commented Sep 9, 2024 •

edited

Loading

mofojed left a comment •

edited

Loading

codecov bot commented Sep 13, 2024 •

edited

Loading

mofojed Sep 17, 2024 •

edited

Loading