Add lint workflow by notypecheck · Pull Request #601 · novnc/websockify

notypecheck · 2025-03-12T18:00:08Z

Closes #600

This PR adds:

Package Manager

https://docs.astral.sh/uv/
Tool to manage project dependencies, with support for pyproject.toml (https://peps.python.org/pep-0621/)

Mypy

https://github.com/python/mypy
Static type checker

Ruff

https://docs.astral.sh/ruff/
Linter and formatter

workflows/lint.yaml uses python 3.12, I don't think this should be an issue as long as target versions are configured properly for ruff and mypy.

notypecheck · 2025-03-12T18:02:05Z

Latest versions of mypy and ruff only support python 3.8 and 3.7 respectively 🤔

d0tiKs · 2025-03-12T21:13:04Z

Latest versions of mypy and ruff only support python 3.8 and 3.7 respectively 🤔

Comparing to novnc compatibility you mean ?

What versions of Python does Ruff support?
Ruff can lint code for any Python version from 3.7 onwards, including Python 3.13.
[...]
Ruff is installable under any Python version from 3.7 onwards.

And for mypy it seems to be : Requires: Python >=3.9
https://pypi.org/project/mypy/

notypecheck · 2025-03-13T00:56:27Z

Comparing to novnc compatibility you mean ?

Yes, currently latest release of websockify on pypi states it's compatible with 3.4 and on master branch it's 3.6.
As for mypy - it's possible to use slightly older version, I think that's already handled in uv.lock:

[package.dev-dependencies]
dev = [
    { name = "mypy", version = "0.971", ..., marker = "python_full_version < '3.7'" },
    { name = "mypy", version = "1.4.1", ..., marker = "python_full_version == '3.7.*'" },
    { name = "mypy", version = "1.14.1", ..., marker = "python_full_version == '3.8.*'" },
    { name = "mypy", version = "1.15.0", ..., marker = "python_full_version >= '3.9'" },
]

only quite early version of ruff (0.0.17) supports 3.6 though

notypecheck · 2025-03-13T00:58:14Z

Also it seems like only ~3% of downloads are for python 3.6 and 3.7 https://pypistats.org/packages/websockify

notypecheck · 2025-03-16T19:18:04Z

@ThinLinc-Zeijlon Sorry to ping you, but I'm not quite sure who maintains the repository right now, but I see that the last commit/release was made by you.
Would you have a bit of time to glance over this PR? Last commit (the one that runs ruff format) can probably be rolled back to reduce review time, and one of the maintainers could run it locally.

CendioZeijlon · 2025-04-14T12:07:35Z

@ThirVondukr, Thank you for your patience, I'll take a look at your changes 🙂

CendioZeijlon · 2025-04-14T13:52:51Z

.gitignore

+
+# Intellij IDEs
+.idea


Local settings folders/files shouldn't be added to the repo.

I thought intellij-based ide's are kind of common and adding that to .gitignore made sense, so contributors don't have do do that themselves. I can remove that if you want though.

CendioZeijlon · 2025-04-14T13:58:30Z

pyproject.toml

+    "jwcrypto>=1.5.1",
+    "numpy>=1.19.0",
+    "redis>=4.3.0",
+    "requests>=2.27.0",


I'm not sure about targeting versions here, where did these come from?

They're probably the latest compatible releases with 3.7, I don't quite remember if I pulled these manually or uv did it.

CendioZeijlon · 2025-04-14T14:01:10Z

uv.lock

Since lock-files are meant for maintaining a local state, we don't want to commit these.

I think lock file is fine to have in a repo. Usually if the project is deployed somewhere (which isn't the case here) you'd want all builds to be reproducible, so lock file helps with that, but it can be used for CI there. (Also you won't really want for dev dependencies to be "automatically" updated either).

CendioZeijlon · 2025-04-14T14:21:27Z

.github/workflows/lint.yaml

+          - ruff format --check .
+          - ruff check .


We'd like to maintain support for older systems. Could you check how far we could get with something more tried and tested like flake8 and pylint?

I didn't use these for quite a while, and I didn't use flake8 since migrating to ruff around a year ago. It should include majority of flake8 and some of the pylint checks though.

CendioZeijlon · 2025-04-14T14:23:03Z

.github/workflows/lint.yaml

+        with:
+          python-version: ${{ matrix.python-version }}
+
+      - name: Install uv


Same as above.

CendioZeijlon · 2025-04-14T14:26:38Z

setup.py

Shouldn't setup.py be removed if we are converting to pyproject.toml?

Yes, it could be removed later.

CendioZeijlon · 2025-04-14T14:28:18Z

tests/echo.py

+            if cqueue or c_pend:
+                wlist.append(self.request)


This is a nice improvement!

CendioZeijlon · 2025-04-14T14:29:49Z

tests/echo.py

@@ -1,23 +1,26 @@
 #!/usr/bin/env python

-'''


I think this is a bit too strict. Can the linter be configured to ignore these types of changes?

Yes, that can be configured to preserve quotes, but the default style is using ", I personally prefer it that way.
https://docs.astral.sh/ruff/settings/#format_quote-style

Personally I don't think it matters that much, having one quote style is better in my opinion, plus developers still can use whatever quote character they want, it would just get reformatted later, but choice is up to you on that.

CendioZeijlon · 2025-04-14T14:31:34Z

tests/echo.py

                if closed:
                    break

-if __name__ == '__main__':


In general, we want the choice of quotation characters to be up the developer.

CendioZeijlon · 2025-04-14T14:33:49Z

tests/echo.py

+    parser.add_option(
+        "--verbose",
+        "-v",
+        action="store_true",
+        help="verbose messages and per frame traffic",
+    )
+    parser.add_option("--cert", default="self.pem", help="SSL certificate file")
+    parser.add_option(
+        "--key", default=None, help="SSL key file (if separate from cert)"
+    )
+    parser.add_option(
+        "--ssl-only", action="store_true", help="disallow non-encrypted connections"
+    )


I think I can guess the strategy it had when formatting here, but I think this made the code less readable.

I think generally you'd want that code to be either on one line:

parser.add_option("--verbose", "-v", action="store_true", help="verbose messages and per frame traffic")

or like this:

parser.add_option( "--verbose", "-v", action="store_true", help="verbose messages and per frame traffic", )

To force the second style you'd need to add , to the last argument, but I think formatter could be disabled for certain code blocks: https://docs.astral.sh/ruff/formatter/#format-suppression

CendioZeijlon · 2025-06-04T11:35:34Z

@CendioOssman and @samhed, what are your thoughts on adding ruff as a linter for us?

CendioOssman · 2025-06-05T07:30:22Z

I'd prefer something more traditional, like flake8. It'll be more generally available and familiar to people.

notypecheck · 2025-06-10T11:21:08Z

@CendioOssman ruff should include majority of rules from flake8 and it's plugins, you can toggle the rules that you think would be useful: https://docs.astral.sh/ruff/rules/

notypecheck added 3 commits March 12, 2025 20:53

build: migrate from setup.py to pyproject.toml

a994cd8

ci: add lint workflow

4fc7e52

run ruff format .

3eca8ad

CendioZeijlon requested changes Apr 14, 2025

View reviewed changes

Conversation

notypecheck commented Mar 12, 2025

Package Manager

Mypy

Ruff

Uh oh!

notypecheck commented Mar 12, 2025

Uh oh!

d0tiKs commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notypecheck commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notypecheck commented Mar 13, 2025

Uh oh!

notypecheck commented Mar 16, 2025

Uh oh!

CendioZeijlon commented Apr 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CendioZeijlon commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CendioOssman commented Jun 5, 2025

Uh oh!

notypecheck commented Jun 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

d0tiKs commented Mar 12, 2025 •

edited

Loading

notypecheck commented Mar 13, 2025 •

edited

Loading

CendioZeijlon commented Jun 4, 2025 •

edited

Loading