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

Find pyproject from vim relative to current file #1871

Merged
merged 6 commits into from Jun 12, 2021

Conversation

austinglaser
Copy link
Contributor

Candidate fix for #1870. May not be the best way to go about this.

@ichard26 ichard26 added the C: vim Vim plugin label Jan 2, 2021
@JelleZijlstra
Copy link
Collaborator

This has a merge conflict now.

Also, unfortunately most of us maintainers don't know much about vimscript and we'll need some help to review this. Can you explain what @% means?

@godlygeek
Copy link
Contributor

Rebasing this should be trivial; the function that's being patched has just moved from plugin/black.vim to autoload/black.vim, but the relevant code is the same.

@x as an expression in vimscript evaluates to the contents of the x register. The % register stores the name of the current file, so @% is the name of the current file (which may be an empty string, if the current buffer is not associated with a file).

But... OK, both the existing code and the proposed code are broken. The vim.eval() call (whether it's vim.eval("@%") or vim.eval("fnamemodify(getcwd(), ':t')) returns a string, and it passes that string to find_pyproject_toml, which expects a sequence of strings, not a single string, and - since a string is a sequence of single character strings - it gets turned into a list of ridiculous paths. I tested with a file called foo.py, and added a print(path_srcs) into find_project_root, which printed out:

[PosixPath('/home/matt/f'), PosixPath('/home/matt/o'), PosixPath('/home/matt/o'), PosixPath('/home/matt'), PosixPath('/home/matt/p'), PosixPath('/home/matt/y')]                                                                  

So... that's wrong.

This seems to do the trick better than both the existing code and the proposed code:

diff --git a/autoload/black.vim b/autoload/black.vim
index f0357b0..0d93aa8 100644
--- a/autoload/black.vim
+++ b/autoload/black.vim
@@ -139,7 +139,8 @@ def Black():
       print(f'Reformatted in {time.time() - start:.4f}s.')
 
 def get_configs():
-  path_pyproject_toml = black.find_pyproject_toml(vim.eval("fnamemodify(getcwd(), ':t')"))
+  filename = vim.eval("@%")
+  path_pyproject_toml = black.find_pyproject_toml((filename,))
   if path_pyproject_toml:
     toml_config = black.parse_pyproject_toml(path_pyproject_toml)
   else:

This does work for an unnamed buffer, too - we wind up calling black.find_pyproject_toml(("",)), and that winds up prepending the working directory to any relative paths, so "" just gets turned into the current working directory.

Note that find_pyproject_toml needs to be passed a 1-tuple, not a list, because it requires something hashable (thanks to functools.lru_cache being used)

ichard26 and others added 4 commits June 11, 2021 22:44
Both the existing code and the proposed code are broken.
The vim.eval() call (whether it's vim.eval("@%") or
vim.eval("fnamemodify(getcwd(), ':t')) returns a string, and it passes
that string to find_pyproject_toml, which expects a sequence of strings,
not a single string, and - since a string is a sequence of single
character strings - it gets turned into a list of ridiculous paths. I
tested with a file called foo.py, and added a print(path_srcs) into
find_project_root, which printed out:

[
  PosixPath('/home/matt/f'),
  PosixPath('/home/matt/o'),
  PosixPath('/home/matt/o'),
  PosixPath('/home/matt'),
  PosixPath('/home/matt/p'),
  PosixPath('/home/matt/y')
]

This does work for an unnamed buffer, too - we wind up calling
black.find_pyproject_toml(("",)), and that winds up prepending the
working directory to any relative paths, so "" just gets turned into
the current working directory.

Note that find_pyproject_toml needs to be passed a 1-tuple, not a
list, because it requires something hashable (thanks to
functools.lru_cache being used)

Co-authored-by: Matt Wozniski <mwozniski@bloomberg.net>
CHANGES.md Outdated
@@ -12,6 +12,11 @@
- Fix incorrect custom breakpoint indices when string group contains fake f-strings
(#2311)

### Integrations

- The vim plugin now searches the directory containing the current buffer instead of the
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to say "searches upwards from"

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's way better! Thanks!

Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

LGTM

@ichard26
Copy link
Collaborator

ichard26 commented Jun 12, 2021

Given that no one on the core team (and who's active enough) is qualified to review vim PRs, I'm merging under the assumption godlygeek's approval is A-OK.

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

Successfully merging this pull request may close these issues.

None yet

4 participants