Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Ruff Configuration Settings #2215

Merged
merged 13 commits into from
Sep 20, 2024
1 change: 0 additions & 1 deletion babel.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ module.exports = api => {
!isTest ? /\.test.(tsx?|jsx?)$/ : false,
!isTest ? '**/__mocks__/*' : false,
'**/*.scss',
'**/*.d.ts',
].filter(Boolean),
};
};
12 changes: 12 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 10 additions & 5 deletions packages/app-utils/src/storage/LocalWorkspaceStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,18 @@ export class LocalWorkspaceStorage implements WorkspaceStorage {
showEmptyStrings: true,
showNullStrings: true,
showExtraGroupColumn: true,
defaultNotebookSettings: {
notebookSettings: {
mofojed marked this conversation as resolved.
Show resolved Hide resolved
isMinimapEnabled: false,
formatOnSave: false,
ruffSettings: {
isEnabled: true,
// Omit default config so default settings are used if the user never changes them
},
},
webgl: true,
webglEditable: true,
gridDensity: 'regular' as const,
};
gridDensity: 'regular',
} satisfies WorkspaceSettings;
const serverSettings = {
defaultDateTimeFormat: serverConfigValues?.get('dateTimeFormat'),
formatter: [],
Expand Down Expand Up @@ -108,13 +113,13 @@ export class LocalWorkspaceStorage implements WorkspaceStorage {
serverConfigValues,
'showExtraGroupColumn'
),
defaultNotebookSettings:
notebookSettings:
serverConfigValues?.get('isMinimapEnabled') !== undefined
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

? {
isMinimapEnabled: LocalWorkspaceStorage.getBooleanServerConfig(
serverConfigValues,
'isMinimapEnabled'
) as boolean,
),
}
: undefined,
webgl: LocalWorkspaceStorage.getBooleanServerConfig(
Expand Down
10 changes: 9 additions & 1 deletion packages/code-studio/src/AppRoot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import { MonacoUtils } from '@deephaven/console';
import { DownloadServiceWorkerUtils } from '@deephaven/iris-grid';
import MonacoWorker from 'monaco-editor/esm/vs/editor/editor.worker?worker';
import MonacoJsonWorker from 'monaco-editor/esm/vs/language/json/json.worker?worker';
import AppRouter from './main/AppRouter';

// load addional css for playwright docker tests
Expand All @@ -16,7 +17,14 @@ export function AppRoot(): JSX.Element {
window.location.href
)
);
MonacoUtils.init({ getWorker: () => new MonacoWorker() });
MonacoUtils.init({
getWorker: (id: string, label: string) => {
if (label === 'json') {
return new MonacoJsonWorker();
}
return new MonacoWorker();
},
});

// disable annoying dnd-react warnings
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
Expand Down
124 changes: 124 additions & 0 deletions packages/code-studio/src/settings/EditorSectionContent.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
import React, { useCallback, useState } from 'react';
import { useDispatch } from 'react-redux';
import { Switch, ActionButton, Icon, Text } from '@deephaven/components';
import { useAppSelector } from '@deephaven/dashboard';
import { getNotebookSettings, updateNotebookSettings } from '@deephaven/redux';
import { vsSettings } from '@deephaven/icons';
import {
MonacoProviders,
RuffSettingsModal,
RUFF_DEFAULT_SETTINGS,
} from '@deephaven/console';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';

export function EditorSectionContent(): JSX.Element {
const notebookSettings = useAppSelector(getNotebookSettings);
const dispatch = useDispatch();
const {
isMinimapEnabled,
formatOnSave,
ruffSettings = {},
} = notebookSettings;
const { isEnabled: ruffEnabled, config: ruffConfig = RUFF_DEFAULT_SETTINGS } =
ruffSettings;

const handleMinimapChange = useCallback(
(newValue: boolean) =>
dispatch(updateNotebookSettings({ isMinimapEnabled: newValue })),
[dispatch]
);

const handleFormatOnSaveChange = useCallback(
(newValue: boolean) =>
dispatch(updateNotebookSettings({ formatOnSave: newValue })),
[dispatch]
);

const handleRuffEnabledChange = useCallback(
(newValue: boolean) => {
dispatch(
updateNotebookSettings({
ruffSettings: { ...ruffSettings, isEnabled: newValue },
})
);
MonacoProviders.isRuffEnabled = newValue;
MonacoProviders.setRuffSettings(ruffConfig);
},
[dispatch, ruffSettings, ruffConfig]
);

const handleRuffConfigChange = useCallback(
(newValue: Record<string, unknown>) => {
dispatch(
updateNotebookSettings({
ruffSettings: { ...ruffSettings, config: newValue },
})
);
MonacoProviders.setRuffSettings(newValue);
},
[dispatch, ruffSettings]
);

const [val, setVal] = useState(JSON.stringify(ruffConfig, null, 2));

const [isRuffSettingsOpen, setIsRuffSettingsOpen] = useState(false);

const handleRuffSettingsClose = useCallback(() => {
setIsRuffSettingsOpen(false);
}, []);

const handleRuffSettingsSave = useCallback(
(v: Record<string, unknown>) => {
handleRuffConfigChange(v);
setVal(JSON.stringify(v, null, 2));
handleRuffSettingsClose();
},
[handleRuffConfigChange, handleRuffSettingsClose]
);

return (
<>
<div className="app-settings-menu-description">
Customize the notebook editor.
</div>

<div className="form-row pl-1">
<Switch isSelected={isMinimapEnabled} onChange={handleMinimapChange}>
Enable Minimap
</Switch>
</div>
<div className="form-row pl-1">
<Switch isSelected={formatOnSave} onChange={handleFormatOnSaveChange}>
Format on Save
</Switch>
</div>
<div className="form-row pl-1">
<Switch
isSelected={ruffEnabled}
onChange={handleRuffEnabledChange}
marginEnd={0}
>
Enable Ruff Python Linter
</Switch>
</div>
<div className="form-row pl-1">
<ActionButton onPress={() => setIsRuffSettingsOpen(true)}>
<Icon>
<FontAwesomeIcon icon={vsSettings} />
</Icon>
<Text>Customize Ruff Settings</Text>
</ActionButton>
</div>
{isRuffSettingsOpen && (
<RuffSettingsModal
text={val}
isOpen={isRuffSettingsOpen}
onClose={handleRuffSettingsClose}
onSave={handleRuffSettingsSave}
/>
)}
</>
);
}

export default EditorSectionContent;
24 changes: 23 additions & 1 deletion packages/code-studio/src/settings/SettingsMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
dhUserIncognito,
dhUser,
vsTools,
vsFileCode,
} from '@deephaven/icons';
import {
Button,
Expand Down Expand Up @@ -38,6 +39,7 @@ import {
} from './SettingsUtils';
import AdvancedSectionContent from './AdvancedSectionContent';
import ThemeSectionContent from './ThemeSectionContent';
import EditorSectionContent from './EditorSectionContent';

interface SettingsMenuProps {
serverConfigValues: ServerConfigValues;
Expand Down Expand Up @@ -70,6 +72,8 @@ export class SettingsMenu extends Component<

static ADVANCED_SECTION_KEY = 'SettingsMenu.advanced';

static EDITOR_SECTION_KEY = 'SettingsMenu.editor';

static focusFirstInputInContainer(container: HTMLDivElement | null): void {
const input = container?.querySelector('input, select, textarea');
if (input) {
Expand Down Expand Up @@ -274,6 +278,24 @@ export class SettingsMenu extends Component<
<ThemeSectionContent />
</SettingsMenuSection>

<SettingsMenuSection
sectionKey={SettingsMenu.EDITOR_SECTION_KEY}
isExpanded={this.isSectionExpanded(SettingsMenu.EDITOR_SECTION_KEY)}
onToggle={this.handleSectionToggle}
title={
<>
<FontAwesomeIcon
icon={vsFileCode}
transform="grow-4"
className="mr-2"
/>
Editor
</>
}
>
<EditorSectionContent />
</SettingsMenuSection>

<SettingsMenuSection
sectionKey={SettingsMenu.SHORTCUT_SECTION_KEY}
isExpanded={this.isSectionExpanded(
Expand All @@ -283,7 +305,7 @@ export class SettingsMenu extends Component<
<>
<FontAwesomeIcon
icon={vsRecordKeys}
transform="grow-2"
transform="grow-4"
className="mr-2"
/>
Keyboard Shortcuts
Expand Down
2 changes: 1 addition & 1 deletion packages/code-studio/src/settings/SettingsMenuSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ function SettingsMenuSection(props: SettingsMenuSectionProps): ReactElement {
className="btn-collapse-trigger"
onClick={() => onToggle(sectionKey)}
>
<div className="flex-grow-1">{title}</div>
<div className="d-flex align-items-center flex-grow-1">{title}</div>
<div className="flex-shrink-0">
<FontAwesomeIcon
transform={isExpanded ? 'flip-v' : ''}
Expand Down
1 change: 1 addition & 0 deletions packages/code-studio/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ export default defineConfig(({ mode }) => {
},
},
optimizeDeps: {
exclude: ['@astral-sh/ruff-wasm-web'], // Needed so the wasm file works in dev mode
mattrunyon marked this conversation as resolved.
Show resolved Hide resolved
esbuildOptions: {
// Some packages need this to start properly if they reference global
define: {
Expand Down
2 changes: 1 addition & 1 deletion packages/components/scss/BaseStyleSheet.scss
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ button:focus {
}

span.btn-disabled-wrapper {
display: contents;
display: inline-block;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

.btn.disabled,
.btn:disabled {
pointer-events: none;
Expand Down
6 changes: 4 additions & 2 deletions packages/components/src/modal/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ function Modal({
onOpened();
}
},
[onOpened, isOpen]
// eslint-disable-next-line react-hooks/exhaustive-deps
[isOpen]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

);

useEffect(
Expand All @@ -82,7 +83,8 @@ function Modal({
onClosed();
}
},
[onClosed, isOpen]
// eslint-disable-next-line react-hooks/exhaustive-deps
[isOpen]
);

if (isOpen && !element.current) {
Expand Down
3 changes: 2 additions & 1 deletion packages/console/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@
},
"scripts": {
"build": "cross-env NODE_ENV=production run-p build:*",
"build:babel": "babel ./src --out-dir ./dist --extensions \".ts,.tsx,.js,.jsx,.scss\" --copy-files --no-copy-ignored --source-maps --root-mode upward",
"build:babel": "babel ./src --out-dir ./dist --extensions \".ts,.tsx,.js,.jsx\" --source-maps --root-mode upward",
"build:sass": "sass --embed-sources --load-path=../../node_modules ./src:./dist"
},
"dependencies": {
"@astral-sh/ruff-wasm-web": "0.6.4",
"@deephaven/chart": "file:../chart",
"@deephaven/components": "file:../components",
"@deephaven/icons": "file:../icons",
Expand Down
3 changes: 1 addition & 2 deletions packages/console/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,13 @@ export { Console };
export { default as ConsoleInput } from './ConsoleInput';
export { default as SHORTCUTS } from './ConsoleShortcuts';
export { default as ConsoleStatusBar } from './ConsoleStatusBar';
export * from './monaco/MonacoThemeProvider';
export { default as MonacoUtils } from './monaco/MonacoUtils';
export { default as Editor } from './notebook/Editor';
export { default as ScriptEditor } from './notebook/ScriptEditor';
export { default as ScriptEditorUtils } from './notebook/ScriptEditorUtils';
export * from './common';
export * from './command-history';
export * from './console-history';
export * from './monaco';
export { default as LogView } from './log/LogView';
export { default as HeapUsage } from './HeapUsage';
export { default as NewTableColumnTypes } from './csv/NewTableColumnTypes';
Loading
Loading