OpenMS / pyopenms-docs

pyOpenMS readthedocs documentation, additional utilities, addons, scripts, and examples.
https://pyopenms.readthedocs.io
Other
43 stars 50 forks source link

Replace flake with flakeheaven for linting #328

Closed matteopilz closed 1 year ago

matteopilz commented 1 year ago

The pyproject.toml needs to be configured. We can also optionally exclude not linted file types automatically and end the action early https://github.com/tj-actions/changed-files.

jpfeuffer commented 1 year ago

It says missing plugins. And you need to test with some actual changes in this PR

matteopilz commented 1 year ago

I have added the plugins and tested it on push. It still needs to be configured and I don't know which settings we want.

jpfeuffer commented 1 year ago

Any settings are fine for me. @OpenMS/core What do you think about auto-applying linting changes with e.g., https://github.com/adamchainz/blacken-docs

timosachsenberg commented 1 year ago

+1 if it doesn’t break it ;)

matteopilz commented 1 year ago

Does blacken-docs work only for code blocks?

jpfeuffer commented 1 year ago

Yes, what else do you want to lint?

matteopilz commented 1 year ago

The markdown in the rst files, basically apply the suggested changes for errors or warnings from flakeheaven automatically. I guess this would be too insecure.

jpfeuffer commented 1 year ago

I think flakeheaven also just looks at python and pycon code blocks. Rst would need another linter (e.g. rst-lint) but I find that sphinx does a decent job with its warnings.

matteopilz commented 1 year ago

Do we then actually still need flakeheaven, if we correct code blocks anyway?

jpfeuffer commented 1 year ago

If we agree to auto-apply, no, sorry. I was actually assuming that flake heaven can auto-apply.

matteopilz commented 1 year ago

blacken needs pre-commit (https://pre-commit.com/#install) installed on the machine to register a commit and actually change something beforehand. How should we handle this? If we install it with a commit on a runner, it kind of defeats the purpose because nothing will be changed.

jpfeuffer commented 1 year ago

you can also just run it in an action. Pre-commit is just the preferred way for most users, since they want to leave the choice to developers I guess.

jpfeuffer commented 1 year ago

python -m pip install blacken-docs && blacken-docs -l 80 docs/source/*.rst

jpfeuffer commented 1 year ago

That's how I did #333

jpfeuffer commented 1 year ago

By the way, I changed my mind for line length as you can see. BEcause this is for documentation. And it should be visible on mobile as well. No one is developing in there.

jpfeuffer commented 1 year ago

You need to somehow suggest or even directly push the changes. I guess the former (suggesting) is a nice middle ground. And for follow up PRs probably not to huge.

https://github.com/reviewdog/action-suggester

jpfeuffer commented 1 year ago

If you only use pull_request as a trigger, which is fine in this case actually, you will not be able to test it. Unless someone does a PR against YOUR branch. (your action is not on master, against which this PR is made)

jpfeuffer commented 1 year ago

Do a PR with the yml only and I will merge it.

matteopilz commented 1 year ago

I think action-suggester works only on pull_request. I'm currently not sure, if it will work, but we can try it. I'm removing the other files.

matteopilz commented 1 year ago

It didn't work, blacken still exits with an error.

matteopilz commented 1 year ago

It looks like flakeheaven would actually report in the output format, action-suggester expects...

jpfeuffer commented 1 year ago

I doubt it, since it is not able to make any changes. You might be able to comment with the comments. But this is almost useless. You have to go back to the code, search the place where the comments were etc. I just want to click on accept suggestion and be done with it.

jpfeuffer commented 1 year ago

Also: flakeheaven is also bugged: https://github.com/flakeheaven/flakeheaven/issues/165