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

Additional sanity checks #3429

Open
JelleZijlstra opened this issue Dec 10, 2022 · 1 comment
Open

Additional sanity checks #3429

JelleZijlstra opened this issue Dec 10, 2022 · 1 comment
Labels
T: enhancement New feature or request

Comments

@JelleZijlstra
Copy link
Collaborator

Currently, Black checks that the AST of its input and output is the same (except for some narrow exceptions). This is a powerful feature that has caught many bugs (e.g. most recently #3428).

Could we add more similar checks?

  • Comment equivalence. We have had bugs in the past where certain comments get removed. Perhaps we could add a simple check that extracts all comments from the input and output (normalized for leading/trailing whitespace) and asserts that the input and output have the same comments in the same order.
  • Obeying the line length. Black can't always ensure all code fits within the desired line length, but at least it shouldn't make things worse by changing code that previously fit within the line length to now exceed the limit. A simple heuristic could be to count the number of lines that are too long in the input and assert that the number of such lines in the output is no greater. However, it's probably very hard to obey this rule in all cases (what if we fix 1-space indentation to 4-space, and in the process move some code we can't split over the line length?). So we probably can't do this unless we come up with a more sophisticated heuristic.

The downside of adding more safety checks is that it will make Black slower. But if users want speed over safety, they can use --fast already, so if the overhead of a comment equivalence check isn't too bad, I think we should do it.

@JelleZijlstra JelleZijlstra added the T: enhancement New feature or request label Dec 10, 2022
@felix-hilden
Copy link
Collaborator

Yeah could be nice 👌 For line length, we could perhaps consider the non-whitespace portion only. For a given line in a diff, we know the post-formatting indentation and could then compare the line length that it would've produced. Splits would be harder though. Or we could indent in another pass since it's such a fundamental formatting move :P (perhaps let's not)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants