Skip to content
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

[flake8-boolean-trap] Add setting for user defined allowed boolean trap #10531

Merged
merged 5 commits into from Mar 30, 2024

Conversation

augustelalande
Copy link
Contributor

@augustelalande augustelalande commented Mar 23, 2024

Summary

Add a setting extend-allowed-calls to allow users to define their own list of calls which allow boolean traps.

Resolves #10485.
Resolves #10356.

Test Plan

Extended text fixture and added setting test.

Copy link
Contributor

github-actions bot commented Mar 23, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@9128305
Copy link

9128305 commented Mar 23, 2024

Is it possible to also check the import path like it should check "django.db.models.Value" and not just "Value" usage?

"Field".to_string(),
"settings".to_string(),
"deploy".to_string(),
"used".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me if these should be function names, or symbol paths (like ["pydantic", "BaseModel"]).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean it's not clear in the documentation/implementation? Or are you saying you're not sure how we should implement it?

Copy link
Member

Choose a reason for hiding this comment

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

The latter!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think symbol path gives finer control, and is less prone to false negatives. The only downside is that it is less intuitive to users, and may lead to confusion. But I think this should be ok with good documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it to symbol paths if everyone agrees.

Copy link

Choose a reason for hiding this comment

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

is it hard to support both?

Copy link
Member

Choose a reason for hiding this comment

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

Symbol path is a little problematic because it doesn't support methods (e.g., there's no way to identify y in x = pydantic.BaseModel(); x.y()), and methods are a common use-case here.

@augustelalande
Copy link
Contributor Author

@charliermarsh I'm moving the discussion to the main thread.

Given your comment I think you do not want to drop support for simple function calls.

Then there are three options:

  1. Keep things as is with users only defining functions to ignore.
  2. Implement both function names and symbol paths using a single setting. It is then up to us to determine whether the given pattern should be interpreted as a function or symbol path (I think it's not that hard?).
  3. Implement both with two settings, one for symbol path, one for functions.

@augustelalande
Copy link
Contributor Author

I lean more towards 2, but not strongly

@charliermarsh
Copy link
Member

Let me quickly cross-reference with some other settings.

@charliermarsh
Copy link
Member

The two linked examples, at least, seem like they would work with symbol paths. So let's start with that. (extend_immutable_calls works with symbol paths, so symbol paths are more consistent with existing settings.)

@augustelalande
Copy link
Contributor Author

Done

@charliermarsh charliermarsh self-assigned this Mar 29, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) March 30, 2024 00:18
@charliermarsh charliermarsh merged commit 3c48913 into astral-sh:main Mar 30, 2024
17 checks passed
@augustelalande augustelalande deleted the extend-boolean-trap branch March 31, 2024 15:58
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.

FBT003 with Pydantic FBT003: Ignore by default django.db.models.Value
4 participants