Use ruff instead of flake8 #79

gaborbernat · 2023-06-16T18:37:21Z

Fix broken issue link and bump tools
Use ruff instead of flake8

Signed-off-by: Bernát Gábor <[email protected]>

godlygeek

As far as I can tell, you've got a mix of manual changes and automatically generated changes, as well as functional changes and stylistic changes, all rolled into a single commit. This is really hard to review. Can you please do separate commits for each logical change?

godlygeek · 2023-06-16T23:23:59Z

docs/conf.py

@@ -1,13 +1,16 @@
-"""Sphinx configuration file for pytest-memray documentation."""
+"""Sphinx configuration file for pytest-memray documentation."""  # noqa: INP001


Mm. It doesn't have any provision for handling scripts that aren't part of packages? That's annoying...

All python files should be formatted, I don't see why this should be different.

I think you misunderstood me. I'm not arguing that docs/conf.py shouldn't be formatted, I'm complaining that ruff thinks that docs is a Python package, and wants is to create a docs/__init__.py - it doesn't understand that docs/conf.py is a standalone script that's not part of any package.

godlygeek · 2023-06-16T23:25:36Z

docs/conf.py

+if TYPE_CHECKING:
+    from sphinx.application import Sphinx


Why does this need to be moved below an if TYPE_CHECKING:?

Done automatically by ruff, read https://beta.ruff.rs/docs/rules/typing-only-first-party-import/

godlygeek · 2023-06-16T23:41:26Z

docs/conf.py

    here = Path(__file__).parent
    root, exe = here.parent, Path(sys.executable)
    towncrier = exe.with_name(f"towncrier{exe.suffix}")
    cmd = [str(towncrier), "build", "--draft", "--version", "NEXT"]
-    new = check_output(cmd, cwd=root, text=True)
+    new = check_output(cmd, cwd=root, text=True)  # noqa: S603


We should probably ignore this rule globally. I have no idea what it's complaining about.

I think locally is better to be explicit that this instance is allowed but not in general. https://beta.ruff.rs/docs/rules/#flake8-bandit-s shows the rules with explanation.

Yes, I read the rule explanation. It seems to complain about every call to subprocess, as far as I can see. I tested running it with cmd set to a list of hardcoded strings, and it still raised an S603 error. That doesn't seem useful to me - have I missed something?

godlygeek · 2023-06-16T23:46:02Z

docs/demo/test_ok.py

@@ -1,18 +1,18 @@
-from __future__ import annotations
+from __future__ import annotations  # noqa: INP001


If this is in docs/, it's intended for end-user consumption, right? Not just for maintainers? If so, I don't think we should add annotations here to appease our linter. I think we should tell the linter to ignore this file instead.

I don't agree. Why should not we lint documentation files same as the rest?

Because it makes the code harder to read by adding annotations that most of our users won't recognize or care about, which makes it function worse as documentation

godlygeek · 2023-06-16T23:47:33Z

pyproject.toml

 readme.content-type = "text/markdown"
+readme.file = "README.md"


Why was this line moved? Did a tool do this automatically?

Indeed used https://github.com/tox-dev/pyproject-fmt

godlygeek · 2023-06-16T23:49:08Z

pyproject.toml

+  "version",
+]
+dependencies = [
+  "memray>=1.8",


Why are we changing from depending on memray >=1.5 to >=1.8? Is this something that should have already been applied in the past and was missed? If so, can you at least make it a separate commit so we can find it when searching through the git log?

Because the CI is only testing the latest version of memray. Anything else is a guess at best, and a straight lie otherwise. We might as well be explicit about what version we support (aka test with). Explicit better than implicit.

OK, fine. This should be done in a separate commit, so that your reviewer doesn't need to guess what changes were made automatically by a tool and what changes were made manually by you.

pyproject.toml

gaborbernat · 2023-06-17T00:09:56Z

As far as I can tell, you've got a mix of manual changes and automatically generated changes, as well as functional changes and stylistic changes, all rolled into a single commit.

Nope, all was done automatically; minus a very few that ruff required to pass. ruff check --fix automatically applies most of the changes, hard to separate them out 🤷 and probably not worth the effort.

godlygeek · 2023-06-17T01:04:47Z

It's very, very difficult to review this when all in one commit lines have been moved and changed, with some changes having been done manually and others having been done by automated tools. I'd like at least 4 separate commits here:

The configuration changes to start using ruff instead of flake8 and isort
The changes automatically made by ruff check --fix
The changes automatically made by pyproject-fmt
The changes manually made to the versions of our declared dependencies

Plus separate commits for any other manual changes I didn't notice.

gaborbernat · 2023-06-17T01:05:51Z

Let's forget it, too much effort. I'll spend my effort on other projects.

gaborbernat · 2023-06-17T01:27:05Z

Thanks for the review though 🙏

gaborbernat force-pushed the ruff branch from fbfaf00 to 2b192e8 Compare June 16, 2023 18:41

gaborbernat changed the title ~~ruff~~ Use ruff instead of flake8 Jun 16, 2023

Use ruff instead of flake8

b7b8d4e

Signed-off-by: Bernát Gábor <[email protected]>

gaborbernat force-pushed the ruff branch from 2b192e8 to b7b8d4e Compare June 16, 2023 18:41

gaborbernat requested a review from pablogsal June 16, 2023 18:41

gaborbernat assigned pablogsal Jun 16, 2023

gaborbernat requested a review from godlygeek June 16, 2023 18:42

gaborbernat assigned godlygeek Jun 16, 2023

godlygeek requested changes Jun 16, 2023

View reviewed changes

gaborbernat requested a review from godlygeek June 17, 2023 00:14

gaborbernat closed this Jun 17, 2023

gaborbernat deleted the ruff branch June 17, 2023 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ruff instead of flake8 #79

Use ruff instead of flake8 #79

gaborbernat commented Jun 16, 2023

godlygeek left a comment

godlygeek Jun 16, 2023

gaborbernat Jun 17, 2023

godlygeek Jun 17, 2023

godlygeek Jun 16, 2023

gaborbernat Jun 17, 2023

godlygeek Jun 16, 2023

gaborbernat Jun 17, 2023

godlygeek Jun 17, 2023

godlygeek Jun 16, 2023

gaborbernat Jun 17, 2023

godlygeek Jun 17, 2023

godlygeek Jun 16, 2023

gaborbernat Jun 17, 2023

godlygeek Jun 16, 2023

gaborbernat Jun 17, 2023 •

edited

Loading

godlygeek Jun 17, 2023

gaborbernat commented Jun 17, 2023 •

edited

Loading

godlygeek commented Jun 17, 2023

gaborbernat commented Jun 17, 2023 •

edited

Loading

gaborbernat commented Jun 17, 2023

		@@ -1,13 +1,16 @@
		"""Sphinx configuration file for pytest-memray documentation."""
		"""Sphinx configuration file for pytest-memray documentation.""" # noqa: INP001

		@@ -1,18 +1,18 @@
		from __future__ import annotations
		from __future__ import annotations # noqa: INP001

		readme.content-type = "text/markdown"
		readme.file = "README.md"

Use ruff instead of flake8 #79

Use ruff instead of flake8 #79

Conversation

gaborbernat commented Jun 16, 2023

godlygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat commented Jun 17, 2023 • edited Loading

godlygeek commented Jun 17, 2023

gaborbernat commented Jun 17, 2023 • edited Loading

gaborbernat commented Jun 17, 2023

gaborbernat Jun 17, 2023 •

edited

Loading

gaborbernat commented Jun 17, 2023 •

edited

Loading

gaborbernat commented Jun 17, 2023 •

edited

Loading