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

efficient formatting #25

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

noneofyourbusiness1415252
Copy link
Contributor

@noneofyourbusiness1415252 noneofyourbusiness1415252 commented Nov 9, 2023

Switch to ruff, which is much faster than yapf -- even though it can't tell whether the contents of the file have changed, causing extra IO/processing in the case they haven't, it's unbeaten by yapf in my tests. Or revert 3f6ede0 for the option which still uses yapf but without spawning an executable each time.
Also ~fixes https://ask.replit.com/t/replit-randomly-sets-all-tab-indententations-to-double-spaces-despite-preferences/57047.
There's a replit issue that indent disappears on format when the tab preference from the file settings differs from the user settings I noticed it was because I'd been passing a tuple to newText instead of a string. But still, it's kind of a replit issue for not validating the response and causing crazy behaviour.

@noneofyourbusiness1415252 noneofyourbusiness1415252 marked this pull request as ready for review November 10, 2023 23:08
@noneofyourbusiness1415252 noneofyourbusiness1415252 marked this pull request as draft November 10, 2023 23:15
@noneofyourbusiness1415252 noneofyourbusiness1415252 marked this pull request as ready for review November 11, 2023 09:45
@noneofyourbusiness1415252 noneofyourbusiness1415252 changed the title efficient async formatting efficient formatting Nov 11, 2023
@noneofyourbusiness1415252 noneofyourbusiness1415252 changed the title efficient formatting efficient settings-~respecting formatting Nov 11, 2023
@noneofyourbusiness1415252
Copy link
Contributor Author

I've put this fork into a template -- except I realised that the pyright-extended used in repls is actually outdated and uses an old version of ruff from the Nix cache, so I had to modify the pyright-langserver.js to reverse the effect of 9520fac.

@jackyzha0
Copy link
Member

Hey! Thanks for the contribution. Can we split this into two PRs?

  1. Respect formatting (the one in packages/pyright-internal/src/languageServerBase.ts)
  2. Switch to ruff via the WASM module. For this second PR, benchmarks would be nice to see

We also looked at two internally but didn't find a way to pass config in via flags but using WASM is a nice way to get around that! Feel free to ping me for a review once you do that :)

@noneofyourbusiness1415252 noneofyourbusiness1415252 changed the title efficient settings-~respecting formatting efficient settings-respecting formatting Nov 24, 2023
@noneofyourbusiness1415252
Copy link
Contributor Author

noneofyourbusiness1415252 commented Nov 24, 2023

@jackyzha0 not sure how to go about splitting it up: adding tab support in pyright-yapf/index.ts differs between using yapf and ruff. So I just made a PR from 68c5e44. And then do you want me to split that up so that there's one PR for respecting indent type preference, and one for the performance improvements?
As for benchmarks, all I can say is that, looking at the ?debug=1 lsp logs, the slowest cases were in the optimised yapf version, and the fastest cases were in ruff, formatting the same file multiple times on a Core plan repl. ruff best case 103ms, and worst case was 3404ms while I held Ctrl+S, spamming formatting requests. The yapf one simply fails if you spam it, and best case was 202ms.
image

@jackyzha0
Copy link
Member

For the first part, I'm looking for just a PR to edit the CLI flags passed to yapf to respect tabs without all the WASM-ed module stuff. As for optimized yapf vs ruff, let's just go with Ruff and I can review that one separately (this should also include the tab formatting option that the first PR includes but for ruff)

The reason is that it'll likely take a few rounds of back and forth to while we review the latter PR but I think it's important to get a working patch in for tab formatting.

@noneofyourbusiness1415252
Copy link
Contributor Author

the WASM-ed module stuff

to be clear, you mean the napi->cpython FFI stuff?

@jackyzha0
Copy link
Member

I don't see any usages of napi ffi in your PR, just @wasm-fmt/ruff_fmt" and the header patching for fetching the wasm?

@noneofyourbusiness1415252
Copy link
Contributor Author

by that I mean the use of node-calls-python in 68c5e44 for yapf

Copy link
Member

@jackyzha0 jackyzha0 left a comment

Choose a reason for hiding this comment

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

theres a lot of package changes (esp at the root level), can you make sure your package-lock changes are scoped to your changes?

packages/pyright/langserver.index.js Outdated Show resolved Hide resolved

This comment has been minimized.

@noneofyourbusiness1415252 noneofyourbusiness1415252 force-pushed the patch-2 branch 2 times, most recently from 55ce798 to 3f6ede0 Compare December 2, 2023 18:40
add tabs support too

fixed!

actually fixed now bruh I'd accidentally deleted the newer clone

switch to Ruff for better perf

Revert "switch to Ruff for better perf"

This reverts commit 76d9c8e.

fix newText bug in yapf option
@noneofyourbusiness1415252 noneofyourbusiness1415252 force-pushed the patch-2 branch 3 times, most recently from 60cca17 to 6ea6f08 Compare December 2, 2023 19:18

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@noneofyourbusiness1415252 noneofyourbusiness1415252 changed the title efficient settings-respecting formatting efficient formatting Dec 2, 2023

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@blast-hardcheese
Copy link
Contributor

Hey there! In an effort to get some confidence around changes like this I added some LSP tests in #32 and #33 which cover completion, linting, and formatting.

I know this PR has been languishing for a while, but I think there's actually either a bug in the upstream ruff wrapper, or a bug in how we're loading the WASM bundle:

$ mkdir python_workspace
$ echo 'print("hello")' > python_workspace/main.py
$ node packages/pyright/dist/pyright.js ./python_workspace/
.../python_workspace/main.py
  .../python_workspace/main.py:1:1 - error: "print" is not defined (pyright[reportUndefinedVariable])
1 error, 0 warnings, 0 informations 

Additionally, in order to test this PR I had to delete

                {
                    test: /\.wasm$/,
                    type: 'asset/inline',
                },

from webpack.config.js, since if that's in there I get TypeError: The URL must be of scheme file, since the entire wasm file gets inlined as data:..., then passed into fs.readFile in recent versions of ruff_fmt. Just an FYI in case you rebase this and try to build.

To run the new test suite, make build test.

Copy link
Contributor

@blast-hardcheese blast-hardcheese left a comment

Choose a reason for hiding this comment

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

I think this is moving in the right direction, but it can't be merged as is unfortunately. Going to mark this as "Request changes," but I'm also happy to either close this out due to the upstream bug(s?) or just switch to invoking ruff on the CLI instead of using the WASM bundle.

Please just let me know how you'd like to proceed, I'd like to get this in!

@noneofyourbusiness1415252
Copy link
Contributor Author

I'd got rid of the ruff_fmt package on my end (and even the ruff CLI for ruff check) in favour of ruff_wasm, but I have yet to create a build script, and I've now got bigger priorities + a clumsy routine. I don't even know whether it works right now.

@rob32
Copy link

rob32 commented Apr 21, 2024

ruff as formatter would be really great. are there any news on this?

@noneofyourbusiness1415252
Copy link
Contributor Author

Try my template. To keep the repl updated properly with the latest features from the replit python module, ruff would have to go directly in pyright-extended, hence this PR. (an alternative would be to patch the latest version of the replit module to use ruff similarly to https://replit.com/@UMARismyname/black?v=1#.config/bashrc, but that would be hard to implement especially if you're going for optimal formatting performance).
As you can see above, there are some upstream issues which mean that I have to rely on patching the ruff_wasm crate instead of simply using the ruff_fmt npm package. I happened to have already started this in the pyright-extended folder @ https://replit.com/@UMARismyname/black#pyright-extended/.git/hooks/post-merge while I was trying to optimise the linting. I've lost interest in programming on replit, so feel free to try and complete the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants