feat: go-to-definition with cmd+click #1425

mscolnick · 2024-05-21T01:01:23Z

Pull Request Template

📝 Summary

This adds go-to-definition with cmd+click

🔍 Description of Changes

Uses a codemirror decorator with underline and builds off some existing go-to-definition logic we have. Most of this work is just the codemirror plugin and some tests.

🛠️ Additional Information

📋 Checklist Before Submitting

I have read the contributor guidelines and the Pull Request section.
This PR fixes an issue (If applicable, specify issue number #).
This change was discussed or approved through an issue or the community discussions (Please provide a link if applicable).
I have added necessary tests for the changes made.
I have run the code and verified that it works as expected.

🧩 Related Issues

Further fixes #1126

📜 Who Can Review?

@akshayka

vercel · 2024-05-21T01:01:27Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marimo-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 21, 2024 0:49am
marimo-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 21, 2024 0:49am

sourcery-ai

Hey @mscolnick - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 7 issues found
🟢 Security: all looks good
🟡 Testing: 1 issue found
🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

sourcery-ai · 2024-05-21T01:03:07Z

frontend/src/core/codemirror/find-replace/search-highlight.ts

 import { store } from "@/core/state/jotai";
 import { findReplaceAtom } from "./state";
 import { getAllEditorViews } from "@/core/cells/cells";
-import { syntaxTree } from "@codemirror/language";


suggestion: Unused import removed

The removal of the syntaxTree import is appropriate since it is no longer used in this file. This helps in keeping the code clean and avoiding unnecessary imports.

sourcery-ai · 2024-05-21T01:03:07Z

frontend/src/core/cells/cells.ts

-      beforeAdjustedCursorPos,
-    );
-    const afterCursorCode = getEditorCodeAsPython(
+    const { beforeCursorCode, afterCursorCode } = splitEditor(


suggestion: Refactored splitCell function

The refactoring of the splitCell function to use splitEditor is a good improvement. However, ensure that splitEditor handles all edge cases correctly, especially with different cursor positions and document states.

Suggested change

const { beforeCursorCode, afterCursorCode } = splitEditor(

const { beforeCursorCode, afterCursorCode } = splitEditor(

cellHandle.editorView,

beforeAdjustedCursorPos,

afterAdjustedCursorPos,

);

sourcery-ai · 2024-05-21T01:03:07Z

frontend/src/core/cells/scrollCellIntoView.ts

@@ -1,10 +1,10 @@
 /* Copyright 2024 Marimo. All rights reserved. */
-import { RefObject } from "react";
+import type { RefObject } from "react";


suggestion: Type import for RefObject

Using import type for RefObject is a good practice to ensure that only the type information is imported, which can help with tree-shaking and reducing bundle size.

sourcery-ai · 2024-05-21T01:03:07Z

frontend/src/core/codemirror/go-to-definition/underline.ts

@@ -0,0 +1,157 @@
+/* Copyright 2024 Marimo. All rights reserved. */


question: New underline feature

The new underline feature for variable names when the meta key is pressed is a useful addition. However, it would be helpful to have more documentation or comments explaining the rationale behind certain design choices, such as the use of requestAnimationFrame in the click method.

sourcery-ai · 2024-05-21T01:03:07Z

frontend/src/core/codemirror/go-to-definition/utils.ts

@@ -0,0 +1,104 @@
+/* Copyright 2024 Marimo. All rights reserved. */
+import type { EditorState } from "@codemirror/state";


suggestion: Utility functions for go-to-definition

The utility functions for go-to-definition are well-structured. Consider adding more unit tests to cover edge cases, such as handling private variables and variables declared in different cells.

sourcery-ai · 2024-05-21T01:03:08Z

frontend/src/core/codemirror/go-to-definition/commands.ts

+  view: EditorView,
+  variableName: string,
+): boolean {
+  const state = view.state;


suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change

const state = view.state;

const {state} = view;

Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

sourcery-ai · 2024-05-21T01:03:08Z

frontend/src/core/codemirror/go-to-definition/underline.ts

+    const cursor: TreeCursor = tree.cursorAt(pos);
+
+    if (cursor.name === "VariableName") {
+      const from = cursor.from;


suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change

const from = cursor.from;

const {from} = cursor;

Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

sourcery-ai · 2024-05-21T01:03:08Z

frontend/src/core/codemirror/go-to-definition/underline.ts

+
+    if (cursor.name === "VariableName") {
+      const from = cursor.from;
+      const to = cursor.to;


suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change

const to = cursor.to;

const {to} = cursor;

Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

sourcery-ai · 2024-05-21T01:03:08Z

frontend/src/core/codemirror/go-to-definition/utils.ts

+  const focusCellId = variable.declaredBy[0];
+  return focusCellId;


suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)

Suggested change

const focusCellId = variable.declaredBy[0];

return focusCellId;

return variable.declaredBy[0];

Explanation
Something that we often see in people's code is assigning to a result variable
and then immediately returning it.

Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.

Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.

sourcery-ai · 2024-05-21T01:03:08Z

frontend/src/core/codemirror/go-to-definition/utils.ts

+ * @param view The editor view at which the command was invoked.
+ */
+export function goToDefinitionAtCursorPosition(view: EditorView): boolean {
+  const state = view.state;


suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change

const state = view.state;

const {state} = view;

Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

akshayka

Wow — this is awesome!!

Maybe we add a line in guides > editor features about how to use this feature?

github-actions · 2024-05-21T13:24:55Z

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.6.1-dev20

This reverts commit 69bd014.

mscolnick added 3 commits May 20, 2024 18:19

feat: go to def

9b8a053

add test for splitCell

4e0f3dd

feat: go-to-definition with cmd+click

4f3b3bf

mscolnick requested a review from akshayka May 21, 2024 01:01

mscolnick added the merge when ready label May 21, 2024

vercel bot deployed to Preview – marimo-storybook May 21, 2024 01:02 View deployment

sourcery-ai bot reviewed May 21, 2024

View reviewed changes

lint, test fix

06f6bb6

vercel bot deployed to Preview – marimo-storybook May 21, 2024 01:25 View deployment

vercel bot deployed to Preview – marimo-docs May 21, 2024 01:27 View deployment

akshayka previously approved these changes May 21, 2024

View reviewed changes

docs test

fda485f

mscolnick dismissed akshayka’s stale review via fda485f May 21, 2024 12:47

vercel bot deployed to Preview – marimo-storybook May 21, 2024 12:49 View deployment

vercel bot deployed to Preview – marimo-docs May 21, 2024 12:49 View deployment

mscolnick merged commit 69bd014 into main May 21, 2024
30 checks passed

mscolnick deleted the ms/start-of-go-to-def branch May 21, 2024 13:19

mscolnick mentioned this pull request May 21, 2024

go to declaration does not work within def #1430

Open

akshayka added a commit that referenced this pull request May 26, 2024

Revert "feat: go-to-definition with cmd+click (#1425)"

3ce403f

This reverts commit 69bd014.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: go-to-definition with cmd+click #1425

feat: go-to-definition with cmd+click #1425

mscolnick commented May 21, 2024

vercel bot commented May 21, 2024 •

edited

sourcery-ai bot left a comment

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

sourcery-ai bot May 21, 2024

akshayka left a comment

github-actions bot commented May 21, 2024

		@@ -0,0 +1,157 @@
		/* Copyright 2024 Marimo. All rights reserved. */

		@@ -0,0 +1,104 @@
		/* Copyright 2024 Marimo. All rights reserved. */
		import type { EditorState } from "@codemirror/state";

		const focusCellId = variable.declaredBy[0];
		return focusCellId;

feat: go-to-definition with cmd+click #1425

feat: go-to-definition with cmd+click #1425

Conversation

mscolnick commented May 21, 2024

Pull Request Template

📝 Summary

🔍 Description of Changes

🛠️ Additional Information

📋 Checklist Before Submitting

🧩 Related Issues

📜 Who Can Review?

vercel bot commented May 21, 2024 • edited

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 21, 2024

Choose a reason for hiding this comment

akshayka left a comment

Choose a reason for hiding this comment

github-actions bot commented May 21, 2024

vercel bot commented May 21, 2024 •

edited