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

Use black for loading black config #1950

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

jeremyheiler
Copy link
Contributor

@jeremyheiler jeremyheiler commented Dec 5, 2021

PR Summary

Fixes #1949

This changes how black configuration is loaded. Instead of looking for
pyproject.toml directly, use a black to load the configuration in the
same way the black CLI would. This allows for other configuration
files to be considered, such as ~/.black or ~/.config/black.

PR checklist

Please make sure that the following things have been addressed (and check the relevant checkboxes):

  • Commits respect our guidelines
  • Tests are passing properly (see here on how to run Elpy's tests)

@jeremyheiler jeremyheiler changed the title Use black module for loading black config Use black config loader for loading black config Dec 5, 2021
@gopar
Copy link
Collaborator

gopar commented Dec 6, 2021

Hey! Many thanks for the PR :)

Looks like black.files did not always exist until:
psf/black@f2ea461 (which was earlier this year)

The function find_pyproject_toml was instead in black/__init__.py. So for projects using an older version of black, this would break what we have for them :/

I do like the idea of letting black find the config file instead of whats happening in right now.

ASK:
Can you make a check that find_pyproject_toml exists in black and if it doesn't then import black.files? I feel this will less likely break things (ah the joys of trying not to break anyone's working elpy config :)

@jeremyheiler
Copy link
Contributor Author

Can you make a check that find_pyproject_toml exists in black and if it doesn't then import black.files? I feel this will less likely break things (ah the joys of trying not to break anyone's working elpy config :)

Yes! I was actually thinking about this last night as I was falling asleep.

@gopar
Copy link
Collaborator

gopar commented Dec 6, 2021

Once the changes are implemented, I'll gladly merge!

@jeremyheiler
Copy link
Contributor Author

All set!

current_version = parse_version(black.__version__)
if current_version >= parse_version("21.5b1"):
from black.files import find_pyproject_toml
elif current_version >= parse_version("20.8b0"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did find_project_toml not exist before this version?
If it didn't then we should have find_project_toml = None as an else branch so that we don't throw an error later down the road since find_project_toml won't be defined.

Copy link
Contributor Author

@jeremyheiler jeremyheiler Dec 7, 2021

Choose a reason for hiding this comment

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

Yes, it was introduced in May of 2020, and the next release was in August 2020. I'll set it to None in the else block. My original implementation defined a custom find_pyproject_toml in the else block, but due to the function signature, it was more complex than adding a check at the call site.

@jeremyheiler jeremyheiler force-pushed the black-config-lookup-fix branch 2 times, most recently from 1b660a5 to b3be4b5 Compare December 7, 2021 01:07
@jeremyheiler jeremyheiler changed the title Use black config loader for loading black config Use black for loading black config Dec 7, 2021
@coveralls
Copy link

Coverage Status

Coverage decreased (-8.7%) to 82.863% when pulling b3be4b5 on jeremyheiler:black-config-lookup-fix into 8d0de31 on jorgenschaefer:master.

This changes how black configuration is loaded. Instead of looking for
`pyproject.toml` directly, use a black to load the configuration in
the same way the black CLI would. This allows for other configuration
files to be considered, such as `~/.black` or `~/.config/black`.
@gopar gopar merged commit a7c72a4 into jorgenschaefer:master Dec 8, 2021
@gopar
Copy link
Collaborator

gopar commented Dec 8, 2021

Merged. Thank You!

@jeremyheiler jeremyheiler deleted the black-config-lookup-fix branch December 8, 2021 16:20
@posita
Copy link
Contributor

posita commented Jan 12, 2022

This probably needs to be reverted or fixed. I'm pretty sure it is the root cause of #1955, which is currently being experienced in elpy-20211211.2248 on Melpa.

@gopar
Copy link
Collaborator

gopar commented Jan 12, 2022

@jeremyheiler do you have some time to look into this for a potential fix?

@@ -43,7 +51,10 @@ def fix_code(code, directory):
# Get black config from pyproject.toml
line_length = black.DEFAULT_LINE_LENGTH
string_normalization = True
pyproject_path = os.path.join(directory, "pyproject.toml")
if find_pyproject_toml:
pyproject_path = find_pyproject_toml((directory,))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may be the root cause of #1955. It looks like (at least with black==21.12b0 that find_pyproject_toml will return None if it can't find "pyproject.toml" in directory.

Calling os.path.exists(pyproject_path) then equates to os.path.exists(None), which will raise an exception:

>>> import os.path
>>> os.path.exists(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/MacPorts-20211130/Python.framework/Versions/3.9/lib/python3.9/genericpath.py", line 19, in exists
    os.stat(path)
TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType

if find_pyproject_toml:
pyproject_path = find_pyproject_toml((directory,))
else:
pyproject_path = os.path.join(directory, "pyproject.toml")
if toml is not None and os.path.exists(pyproject_path):
Copy link
Contributor

@posita posita Jan 12, 2022

Choose a reason for hiding this comment

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

I think #1955 can be fixed by changing this line to:

if toml and pyproject_path and os.path.exists(pyproject_path):

This seems to cure the problem in my hand-testing of both in 0803e76 and master, so I think it should do the trick. 👌

@posita
Copy link
Contributor

posita commented Jan 12, 2022

@jeremyheiler do you have some time to look into this for a potential fix?

Yup! See this comment for a recommendation. I'm pretty sure that should do it. UPDATE: PR is up (#1956), but I could use help testing it.

You'll have to forgive me, past that, though. I don't have much experience with elisp or elpy's test mechanisms to formulate an automated way to repro this, so I think that's about as useful as I can be in this context (at least right now).

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.

elpy-black-fix-code does not use black config file
4 participants