Parse YAPF diffs into TextEdits (instead of sending the full doc) #136

masad-frost · 2021-12-09T09:31:49Z

Currently formatting with YAPF sends back the full document as a range, it's not great because:

Editors that allow edits while formatting is pending cannot reasonably consolidate the format diff with the user's local changes
Editors that have to talk to servers now have to send the whole document back and forth (admittedly, for smaller documents the added JSON structures offset that)
The person who built the plugin wanted to do it, but couldn't be bothered 😆
In general, it is the recommended approach by the spec

To support this feature I used YAPF's unified diff output option. I took the output and passed it to whatthepatch to produce a parsed result, the format is outlined in the library's readme. We iterate over lines in the diff and create TextEdits. Left some comments around the code to make it more readable.

To be able to test without strictly matching TextEdits I also added a function to the workspace module called apply_text_edits which takes a Document instance and TextEdits and returns a new source/string with the text edits applied. I wrote tests for this function based on the tests in the vscode language server repo. This function is now only used for testing, but I imagine it being useful for anyone working with text edits

krassowski · 2021-12-11T12:58:58Z

Thanks for starting this work!

The tests are failing on CI - it looks like they need updating to match the changes in 341732d:

>       assert doc.apply_text_edits(res) == "A = ['h', 'w', 'a']\n\nB = ['h', 'w']\n"
E       AttributeError: 'Document' object has no attribute 'apply_text_edits'

masad-frost · 2022-03-10T03:15:15Z

@krassowski the tests pass locally now if you wanna run the workflow.

I remember doing this and running tests!

masad-frost · 2022-03-17T20:51:24Z

Actually fixed now. Ran pytest and pylint

masad-frost · 2022-03-22T18:53:42Z

Weird, I'm not getting these lint issues locally. Will look into it

masad-frost · 2022-03-30T19:07:23Z

Will merge main after #134 is merged

pylsp/workspace.py

ccordoba12 · 2022-04-05T15:49:43Z

@masad-frost, now that #134 is merged, you can fix the merge conflicts here so we can merge it.

ccordoba12

Hey @masad-frost, thanks for patience with this! Last comment, then this should be ready.

Also, please fix the merge conflicts that this PR has now.

pylsp/workspace.py

pylsp/_utils.py

masad-frost · 2022-06-02T21:42:32Z

pylsp/plugins/yapf_format.py

+    return text_edits
+
+
+def ensure_eof_new_line(document, eol_chars, text_edits):


This is a bit of a hack, see: google/yapf#1008

masad-frost · 2022-06-02T21:58:41Z

@ccordoba12 lint issues seem to be unrelated from this branch.

ccordoba12

Hey @masad-frost, thanks for the updates! I think you're really close to finish this one.

pylsp/_utils.py

pylsp/text_edit.py

test/test_text_edit.py

test/test_workspace.py

ccordoba12 · 2022-06-05T17:55:25Z

@ccordoba12 lint issues seem to be unrelated from this branch.

I'll help with that after you solve my comments.

masad-frost · 2022-06-06T15:57:09Z

thanks, quick fixes esp with the suggestions!

masad-frost · 2022-06-06T16:32:17Z

Just the lints are left! Let me know if there's anything else I can address. If you have more test cases in mind or something

ccordoba12

Thanks for your hard on this @masad-frost! I think it's finally ready.

Changes: * The `test_numpy_completion` test is currently broken - see davidhalter/jedi#1864 * The test flags were moved from setup.cfg to pyproject.toml - see python-lsp/python-lsp-server#207 * A dependency on `whatthepatch` was added for yapf - see python-lsp/python-lsp-server#136

masad-frost force-pushed the fm-yapf-diff branch 2 times, most recently from 6303e3d to b1e3447 Compare March 10, 2022 02:52

masad-frost and others added 7 commits March 17, 2022 13:50

Add apply_text_edits method to workspace document

9b45612

Sendback diffs instead of the whole document with yapf formatting

c7623f6

Remove apply_text_edits from document method

613b173

oops

fe0980d

Fix the test

0565667

Fix not passing doc to apply_text_edit

fa394ad

I remember doing this and running tests!

Lint-y lint

2dbf392

masad-frost force-pushed the fm-yapf-diff branch from 0bb73cc to 2dbf392 Compare March 17, 2022 20:50

ccordoba12 reviewed Apr 5, 2022

View reviewed changes

pylsp/workspace.py Outdated Show resolved Hide resolved

masad-frost mentioned this pull request May 11, 2022

Add support for LSP formatting options parameter #134

Merged

ccordoba12 added this to the v1.5.0 milestone May 28, 2022

ccordoba12 reviewed May 28, 2022

View reviewed changes

pylsp/workspace.py Outdated Show resolved Hide resolved

masad-frost added 2 commits June 2, 2022 11:02

Merge develop

26e977e

Make a separate module for text_edit

8847a5d

masad-frost force-pushed the fm-yapf-diff branch from 24a7a1c to 8847a5d Compare June 2, 2022 18:30

masad-frost added 3 commits June 2, 2022 13:13

handle EOF newline and an extra test

2f9a509

lint

c1f5d2a

Make pylint happier

55f6eab

masad-frost commented Jun 2, 2022

View reviewed changes

pylsp/_utils.py Outdated Show resolved Hide resolved

Match new lines first

89f6211

masad-frost commented Jun 2, 2022

View reviewed changes

Hopefully last lint

a8cd387

masad-frost force-pushed the fm-yapf-diff branch from b33a556 to a8cd387 Compare June 2, 2022 21:53

Update setup.cfg

1849f7d

ccordoba12 reviewed Jun 5, 2022

View reviewed changes

pylsp/_utils.py Outdated Show resolved Hide resolved

pylsp/_utils.py Outdated Show resolved Hide resolved

pylsp/text_edit.py Show resolved Hide resolved

test/test_text_edit.py Show resolved Hide resolved

test/test_workspace.py Outdated Show resolved Hide resolved

masad-frost added 4 commits June 6, 2022 08:58

Undo change to EOL_CHARS

06f5e34

Undo changes to get_eol_chars

1fb8ff2

Add back new line

c1daaf8

Add copyright headers

5196079

ccordoba12 added 3 commits June 6, 2022 12:10

Fix linting issues

ea6fc90

More linting fixes

c404a51

Fix pycodestyle issue

53c1caa

ccordoba12 approved these changes Jun 6, 2022

View reviewed changes

ccordoba12 merged commit a9d503c into python-lsp:develop Jun 6, 2022

masad-frost deleted the fm-yapf-diff branch June 6, 2022 18:50

lilyinstarlight mentioned this pull request Jul 30, 2022

python3Packages.python-lsp-server: fix test issues from 1.5.0 upgrade NixOS/nixpkgs#184026

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse YAPF diffs into TextEdits (instead of sending the full doc) #136

Parse YAPF diffs into TextEdits (instead of sending the full doc) #136

masad-frost commented Dec 9, 2021

krassowski commented Dec 11, 2021

masad-frost commented Mar 10, 2022

masad-frost commented Mar 17, 2022

masad-frost commented Mar 22, 2022

masad-frost commented Mar 30, 2022

ccordoba12 commented Apr 5, 2022

ccordoba12 left a comment

masad-frost Jun 2, 2022

masad-frost commented Jun 2, 2022

ccordoba12 left a comment

ccordoba12 commented Jun 5, 2022

masad-frost commented Jun 6, 2022

masad-frost commented Jun 6, 2022 •

edited

Loading

ccordoba12 left a comment

		return text_edits


		def ensure_eof_new_line(document, eol_chars, text_edits):

Parse YAPF diffs into TextEdits (instead of sending the full doc) #136

Parse YAPF diffs into TextEdits (instead of sending the full doc) #136

Conversation

masad-frost commented Dec 9, 2021

krassowski commented Dec 11, 2021

masad-frost commented Mar 10, 2022

masad-frost commented Mar 17, 2022

masad-frost commented Mar 22, 2022

masad-frost commented Mar 30, 2022

ccordoba12 commented Apr 5, 2022

ccordoba12 left a comment

Choose a reason for hiding this comment

masad-frost Jun 2, 2022

Choose a reason for hiding this comment

masad-frost commented Jun 2, 2022

ccordoba12 left a comment

Choose a reason for hiding this comment

ccordoba12 commented Jun 5, 2022

masad-frost commented Jun 6, 2022

masad-frost commented Jun 6, 2022 • edited Loading

ccordoba12 left a comment

Choose a reason for hiding this comment

masad-frost commented Jun 6, 2022 •

edited

Loading