zfit / phasespace

Phase space generation implemented in TensorFlow
https://phasespace.rtfd.io
BSD 3-Clause "New" or "Revised" License
24 stars 9 forks source link

ci: add and enforce flake8 and prettier #52

Closed redeboer closed 3 years ago

redeboer commented 3 years ago

Noticed that flake8 is listed as a dev dependency, but is not enforce. This fixes that.

While I was at it, I added Prettier as a hook to format markdown, yaml etc. and cleaned up the pre-commit config with some meta hooks.

redeboer commented 3 years ago

Tbh, I have mixed feelings on both these tools:

  • enforcing flake8 we discussed also internally long time ago. It yielded too often false positives or needs the # noqa. With black and pre-commits as well as a decent IDE I found to make less mistakes by not enforcing it, got used to ignore the messages. But we can oc try it out and see, maybe I don't remember it well anyway

Ah I see. Maybe did this PR, because I noticed a remark about flake8 in the contribute notes and it was listed in the dev requirements. Personally I have good experiences with flake8 (better than with pylint, and not so many false positives), but could be because it improved over time and I joined in on python later. Here, it helped me pick out some unused imports for instance.

  • prettier is a two-sided sword: it is great if it works but can also make damage: the double backticks of code in rst. So my fear is that is removes them (as it should be double backtick in rst). But we can see

I like prettier for markdown and yaml, but indeed it's not crucial. I don't think it operates on rst or python by default (requires plugins), but if so, that can be disabled in the hook.

But fine, let's merge it and see, won't be too bad, maybe I made mistakes when using them in the past, so go ahead with the merge

jonas-eschle commented 3 years ago

I like prettier for markdown and yaml, but indeed it's not crucial. I don't think it operates on rst or python by default (requires plugins), but if so, that can be disabled in the hook.

Indeed, it should not, but for the docs I think it did not realize that it wasn't markdown but rst.

But sure, lets see, in general I am a strong advocate of any automatic tool! (as long as they don't do damage)