lxml / lxml-stubs

Type stubs for the lxml package
Other
43 stars 29 forks source link

Suggestion: disable format check in PR and normal commits #59

Closed abelcheung closed 2 years ago

abelcheung commented 2 years ago

Although format checks can help users reading the code with more consistency, the way it's done currently is a burden for both contributor and maintainer alike.

  1. In particular, it insists on changing function / method layout every now and then, making patch harder to read.
  2. Besides, format error in github workflow worker can potentially hide typing error. If format error is found, mypy could stop checking as well, depending on the execution order.

mypy check is much more important, preventing code error to manifest in annotation for too long. Given how lxml-stubs is maintained currently, it's good enough for format check to be performed manually only once, just before tagged release.

scoder commented 2 years ago

Although format checks can help users reading the code with more consistency, the way it's done currently is a burden for both contributor and maintainer alike.

I've had it too many times that PRs wracked the formatting in one way or another and I had to go and fix it later. So, no, for me, it's easier this way. And it preserves the history more accurately than reformatting after a merge would.

1. In particular, it insists on changing function / method layout every now and then, making patch harder to read.

That seems an acceptable price to pay.

2. Besides, format error in github workflow worker can potentially hide typing error. If format error is found, `mypy` could stop checking as well, depending on the execution order.

I just disabled the "fail fast" mode to get all results even if one fails.

JelleZijlstra commented 2 years ago

In typeshed we now use pre-commit.ci to automatically push format fixes to PR branches. That may be useful here too.

abelcheung commented 2 years ago

@JelleZijlstra Not sure about that, as management style and workflow are entirely different. Anyway it would be entirely maintainer's call 😄