I think it was removed for that exact reason? not understanding our stdlib dependencies provided at runtime by JS/WASM interpreters?

@cclauss actually, there is some good hint but stuff like this https://github.com/pyscript/pyscript/pull/2292/files#diff-012bba8176eef49ab8becc504b72a0b285ede3619f1706961a6873169db1402dR5 breaks MicroPython. Our standard library works in both Pyodide (C-Python) and MicroPython and IIRC that's the reason we removed ruff at some point, it was breaking MicroPython in various ways.

I don't mind going through these changes but those that break MicroPython can't land and others are more aesthetic related changes than meaningful one, performance or logic wise speaking ... so ... would it be OK to have bokeh just not mandatory and be used as a random hint around our code so that we can spot and flag or address obvious things and ignore the rest? Otherwise I don't think this will ever pass our CI due MicroPython integration tests and compatibility issues which made us remove it in the first place.

MicroPython itself uses Ruff, maybe we could use their configuration? https://github.com/micropython/micropython/blob/8987b39e0b772d22539022f0961c8ea0f7162751/pyproject.toml#L23

Thanks both. Yes, it took a bit of discussion to get to that basic configuration.

• all: GitHub Action to lint Python code with ruff. micropython/micropython#10977

if we manage to make it work and pass CI I have nothing against it but definitively we should use what MicroPython uses as config, not what Pyodide or regular Python would ... the former is more limited, the latter works as superset.

Let's see what CI says

as side note, I think tests/ folder should be excluded ... there is code literally copy/pasted with copy rights too in there and us changing 3rd party code doesn't look like super useful ... or better, I don't lint 3rd party libraries, usually, and the same goes for flatted.py, it's from the flatted module so I need to fix eventually in there (which I'll do) but it's a bit pointless to have it checked in here.

On top of that, CI is failing with all MicroPython tests ... my proposal to keep it optional and check from time to time remains, if it breaks stuff we carefully tested and covered it's not a good tool, rather a constant "paranoia" to me.

Curious to hear what @ntoll or @fpliger think about this ... after all, most fixes are for the web module, they wrote it, and also dropping repl might have undesired side-effects I don't know on MicroPython.

edit I did update flatted to fix those hints but CI keeps failing with MicroPython

• all: Upgrade to ruff v0.9.6. micropython/micropython#16774

thanks @cclauss let's see how that goes ... meanwhile, CI still failing in here. I'll wait for that one to not fail before investigating possible reasons.

Morning folks, I welcome these changes but I hope we can ensure such checks work with both MicroPython and Pyodide. I'm cautious of over-use of tooling such that we spend an inordinate and unhelpful amount of time trying to fix code-quality edge cases. But if we have a configuration that makes, for example MicroPython, things work smoothly then I see no reason why we should use it. Thanks @cclauss for brining this up, and (as always) to @WebReflection and @danyeaw for thoughtful and important contributions.

Let's make it work.
Then let's make it work properly.

;-)

Also, the test suite is something on my radar at this moment in time. I want to revisit it in the next week or two to investigate and ensure timely and predictable test runs on CI.

Yeah, so checking the code, I see ruff is suggesting things that MicroPython doesn't support. We need to carefully look into this.

% ruff rule FA102 breaks MicroPython -- Missing from __future__ import annotations, but uses PEP 604 union
% ruff rule FURB188 breaks MicroPython which has no str.replaceprefix() or str.replacesuffix().

glad to see CI is green now ... what I'd like to do before approving and merging is to test manually all tests that require manual validation to be sure everything is fine and we can move forward from now on with ruff too, imho ... does that makes sense @ntoll and @fpliger ??? My understanding after yesterday call is that we want this to be the way forward, thanks!

Yes! 🎉

Thank you to everyone (especially @cclauss) for contributing to this!

@cclauss what @ntoll said but an extra question from my side: I'd like to have this as part of our pre-commit CI hook ... by any chance you can tell me how are you running ruff to validate or fix this? I think it should either in entirely, or these one-off PRs might be more tedious than they need to be in the near future, thank you!

Added...

    - repo: https://github.com/astral-sh/ruff-pre-commit
      rev: v0.9.6
      hooks:
        - id: ruff

https://docs.astral.sh/ruff/integrations/#pre-commit

While creating these PRs, I run ruff on the command line. But afterward, it is best to put ruff into pre-commit or CI.

Bravo!

@ntoll and @fpliger I am going to approve this, which will run ruff any time we commit Python code ... thumb up in here to approve that so we can merge it in our project 👍

@cclauss do you see yourself force-pushing anything else? I can wait for that if that's the case

Nothing else. This is ready for review.