Skip to content

Lint code with ruff #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Lint code with ruff #99

wants to merge 1 commit into from

Conversation

nkakouros
Copy link
Contributor

@nkakouros nkakouros commented Jan 17, 2025

Closes #61, #60.

@nkakouros
Copy link
Contributor Author

This will wait for the lang graph branch to be merged, and the auto fixes will be applied again. But in general how does this look like? Is there sth you want to change?

I think having such a setup and configuration will make life a lot easier. For instance, we won't have to think again about formatting, the code will have a consistent look and some bugs may fixed in the mean time.

The drawback of this PR is that despite the auto fixes there are almost 2k issues that need to be resolved.

os.path.dirname(os.path.abspath(__file__)),
"default.conf"
)
CONFIGFILE = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'default.conf')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line length seems too long - I think me and Andrei have line length set to 79 or 80

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can configure it. 88 is the common setting and the default in ruff. If you look at pyproject.toml there is a rationale behind it.

About 88 being the default, Black, which is what ruff follows, says:

Line length

You probably noticed the peculiar default line length. Black defaults to 88 characters per line, which happens to be 10% over 80. This number was found to produce significantly shorter files than sticking with 80 (the most popular), or even 79 (used by the standard library). In general, 90-ish seems like the wise choice.

If you’re paid by the lines of code you write, you can pass --line-length with a lower number. Black will try to respect that. However, sometimes it won’t be able to without breaking other rules. In those rare cases, auto-formatted code will exceed your allotted limit.

It is also the case that if you have a hard limit of 80, then you may have one or two characters that go over it. It is a good practice, and there are plugins that allow for a 10% more line length than the configured one so that you don't have to sacrifice readability to satisfy a linter or a hard rule. 10% of 80 is 88. This may be a coincidence, but I doubt it.

In general, my approach to these things has been to go with what the defaults are, and by defaults, I mean also the tools of the trade such as ruff and black. This is similar to how I don't override application defaults anymore; use and learn what is there instead of learning your own things. This keeps peace of mind and reduces the maintenance burden.

But if you need this changed, it can be changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think 88 is too long. Looking at that line it is too long (more breath with shorter lines).

@nkakouros
Copy link
Contributor Author

I autofixed whatever ruff could fix. I then ignored all failing rules. This can be merged as is now and the ignored rules can be uncommented one by one and the issues addressed in separate PRs.

@nkakouros nkakouros marked this pull request as ready for review February 1, 2025 02:41
@nkakouros
Copy link
Contributor Author

Do I misunderstand sth or are the mypy warnings valid and should have been valid even before the changes in this PR? For example, when the tests did (and still do):

    node1 = AttackGraphNode(
        type = "defense",
        name = "node1",
        lang_graph_attack_step = None,
    )

This indeed conflicts the signature of AttackGraphNode as lang_graph_attack_step is not typed as Optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize order of imports
2 participants