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

Feature/issue 414: vim plugin respects pyproject.toml config #589

Closed
wants to merge 3 commits into from
Closed

Feature/issue 414: vim plugin respects pyproject.toml config #589

wants to merge 3 commits into from

Conversation

pappasam
Copy link
Contributor

@pappasam pappasam commented Oct 29, 2018

This pull request makes the Vim plugin respect pyproject.toml without changing any public interfaces (resolves #414)

I pulled some functionality out of read_pyproject_toml and put it into 2 separate functions: abspath_pyproject_toml and parse_pyproject_toml. These are used by the vim plugin to find and read a relevant pyproject.toml according the the already-established rules of the project.

Regarding tests

I did not yet add tests for this modification; the original function (read_pyproject_toml) was not explicitly tested and I'm unsure what the preferred testing location would be for a bunch of pyproject.toml files.

I did run the tests using the technique recommended in the contributing guidelines and no existing tests break.

Creates two separate functions:

1) abspath_pyproject_toml: find the absolute path to pyproject.toml
2) parse_pyproject_toml: finds black-specific toml config
I thought `pwd` would be sufficient, but needed
fnamemodify(getcwd(), ':t') to get the full cwd path
@coveralls
Copy link

Pull Request Test Coverage Report for Build 787

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.006%) to 96.174%

Files with Coverage Reduction New Missed Lines %
black.py 1 95.2%
tests/test_black.py 1 97.83%
Totals Coverage Status
Change from base Build 786: 0.006%
Covered Lines: 2916
Relevant Lines: 3032

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 787

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.006%) to 96.174%

Files with Coverage Reduction New Missed Lines %
black.py 1 95.2%
tests/test_black.py 1 97.83%
Totals Coverage Status
Change from base Build 786: 0.006%
Covered Lines: 2916
Relevant Lines: 3032

💛 - Coveralls

@dimbleby
Copy link
Contributor

dimbleby commented Nov 7, 2018

would be good to reach a conclusion, one way or the other, on #416 before making very much change to the Vim plugin.

@RohitK89
Copy link

Super excited to get this merged in! I have to stop using the plugin for now because I have project-specific configs which differ from the defaults. I feel like a pleb having to run the command from the CLI. . .

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this! I left a few comments.

@@ -138,6 +138,23 @@ def from_configuration(
return mode


def abspath_pyproject_toml(path_search_start: str) -> Optional[str]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like find_pyproject_toml would be a better name for this function. Also, why does it return a str instead of a Path?

except Exception as exc:
config_pyproject_toml = {}

toml_line_length = config_pyproject_toml.get("line_length")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not great that we have to hardcode all of these options here. Is there any way to avoid having separate code for each option? (Relatedly, #618 will change a few of these.)

path_pyproject_toml = black.abspath_pyproject_toml(vim.eval("fnamemodify(getcwd(), ':t')"))
try:
config_pyproject_toml = black.parse_pyproject_toml(path_pyproject_toml)
except Exception as exc:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Catching any Exception is bad practice. What errors is this looking for specifically?

@pappasam
Copy link
Contributor Author

pappasam commented Feb 5, 2019

@JelleZijlstra no worries! Unfortunately, given the lack of configuration options for black, I've stopped using it. Additionally, given the amount of time I spent looking elsewhere for something that respects the pyproject.toml, I ended up writing my own vim formatting plugin that does everything I want for every language.

Feel free to close this pull request, or use it as a jumping off point in case you're interested in its content. Totally agree with your comments regarding exception handling, and with hardcoding options... see my plugin for a well-functioning alternative on POSIX-like systems and apologies for the rush job.

@anthrotype
Copy link

it looks like this PR is now stale.. any plans to add support for respecting pyproject.toml in the official black.vim plugin?
Or can anybody recommend a good alternative solution?

@pappasam
Copy link
Contributor Author

@anthrotype if you are on a Unix-like system, my auto-formatting plugin has worked well for me. To use black for Python, just use the string "black -q -" when configuring.

https://github.com/pappasam/vim-filetype-formatter

@dimbleby
Copy link
Contributor

Several suggestions in this comment, in which I propose that it doesn't make a lot of sense to maintain a vim plugin in this repository.

Personally I just use formatprg.

@anthrotype
Copy link

Thanks both, I’ll have a look!

@TalAmuyal
Copy link
Contributor

@ambv, @JelleZijlstra, @pappasam, #1273 has been merged in stead of this one.
Please closes this.

@pappasam pappasam closed this Mar 3, 2020
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.

Vim plugin should respect pyproject.toml config
7 participants