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

Insert the cells from the start position #15398

Merged
merged 2 commits into from
Jan 10, 2025
Merged

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Jan 10, 2025

Summary

The cause of this bug is from #12575 which was itself a bug fix but the fix wasn't completely correct.

fixes: #14768
fixes: astral-sh/ruff-vscode#644
fixes: astral-sh/ruff-vscode#573

Test Plan

Consider the following three cells:

class Foo:
    def __init__(self):
        self.x = 1

    def __str__(self):
        return f"Foo({self.x})"
def hello():
    print("hello world")
y = 1

The test case is moving cell 2 to the top i.e., cell 2 goes to position 1 and cell 1 goes to position 2.

Before this fix, it can be seen that the cells were pushed at the end of the vector:

  12.643269917s  INFO ruff:main ruff_server::edit::notebook: Before update: [
    NotebookCell {
        document: TextDocument {
            contents: "class Foo:\n    def __init__(self):\n        self.x = 1\n\n    def __str__(self):\n        return f\"Foo({self.x})\"",
        },
    },
    NotebookCell {
        document: TextDocument {
            contents: "def hello():\n    print(\"hello world\")",
        },
    },
    NotebookCell {
        document: TextDocument {
            contents: "y = 1",
        },
    },
]
  12.643777667s  INFO ruff:main ruff_server::edit::notebook: After update: [
    NotebookCell {
        document: TextDocument {
            contents: "y = 1",
        },
    },
    NotebookCell {
        document: TextDocument {
            contents: "class Foo:\n    def __init__(self):\n        self.x = 1\n\n    def __str__(self):\n        return f\"Foo({self.x})\"",
        },
    },
    NotebookCell {
        document: TextDocument {
            contents: "def hello():\n    print(\"hello world\")",
        },
    },
]

After the fix in this PR, it can be seen that the cells are being pushed at the correct start index:

   6.520570917s  INFO ruff:main ruff_server::edit::notebook: Before update: [
    NotebookCell {
        document: TextDocument {
            contents: "class Foo:\n    def __init__(self):\n        self.x = 1\n\n    def __str__(self):\n        return f\"Foo({self.x})\"",
        },
    },
    NotebookCell {
        document: TextDocument {
            contents: "def hello():\n    print(\"hello world\")",
        },
    },
    NotebookCell {
        document: TextDocument {
            contents: "y = 1",
        },
    },
]
   6.521084792s  INFO ruff:main ruff_server::edit::notebook: After update: [
    NotebookCell {
        document: TextDocument {
            contents: "def hello():\n    print(\"hello world\")",
        },
    },
    NotebookCell {
        document: TextDocument {
            contents: "class Foo:\n    def __init__(self):\n        self.x = 1\n\n    def __str__(self):\n        return f\"Foo({self.x})\"",
        },
    },
    NotebookCell {
        document: TextDocument {
            contents: "y = 1",
        },
    },
]

@dhruvmanila dhruvmanila added bug Something isn't working server Related to the LSP server labels Jan 10, 2025
@AlexWaygood AlexWaygood added the notebook Related to (Jupyter) notebooks label Jan 10, 2025
@dhruvmanila
Copy link
Member Author

dhruvmanila commented Jan 10, 2025

TODO: I'm going to write a test case for this.

Edit: Added a test case for this scenario.

Copy link
Contributor

github-actions bot commented Jan 10, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/ruff_server/src/edit/notebook.rs Outdated Show resolved Hide resolved
@dhruvmanila dhruvmanila enabled auto-merge (squash) January 10, 2025 13:05
@dhruvmanila dhruvmanila force-pushed the dhruv/fix-notebook-update branch from 6f5ac98 to efc032f Compare January 10, 2025 13:06
@dhruvmanila dhruvmanila merged commit 6e9ff44 into main Jan 10, 2025
20 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/fix-notebook-update branch January 10, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working notebook Related to (Jupyter) notebooks server Related to the LSP server
Projects
None yet
3 participants