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

false positive S001 on modulo division #23

Open
amateja opened this issue Dec 28, 2017 · 4 comments
Open

false positive S001 on modulo division #23

amateja opened this issue Dec 28, 2017 · 4 comments

Comments

@amateja
Copy link

amateja commented Dec 28, 2017

Please consider following example:

def modulo(foo: int, bar: int) -> int:
    return foo % bar

Versions 1.1 and 1.2 are affected.

@gforcada
Copy link
Owner

@amateja thanks for the report.

I also show a warning when two variables are used because I often write code like this:

msg = 'Hi %s, how are you doing?'
print(msg % 'John')

Another reason was also to consider that a numerical modulo operation is less frequent than string formatting.

I'm thinking that we could create a new error code (S002?) for your use case, i.e. when the modulo operation is applied to a variable.

This way S001 would be reported whenever a literal string is on the left hand and S002 when is a variable.

Improving the checker to use the type hints would be another idea, I like it! though I guess it is not so much widespread that it we be of little use so far (but it can be already handy for you).

I'm on vacations until January 9th, before and after that I can not make much promises on these two improvements (new error code and type hints), but I will try my best to make a pull request as soon as possible.

Do you have the skills and time to make the necessary changes yourself?

If you have code analysis as part of your build or commit hook, you can always temporarily add a # noqa: S001 on that single line.

@radkujawa
Copy link

Hi!
Three years passed and situation with type hints changed - together with python3 many developers use type hints now.

I use flake8-pep3101 in my project and I love it (we used it to get rid of % formatting completely from the codebase), but this particular bug sometimes forces us to add more # noqa: S001 over the time. I would be great if this bug is resolved, in my opinion the best way to solve it is to support type hints.

@gforcada
Copy link
Owner

gforcada commented Oct 8, 2022

Two more years passed, and so far, no hero showed up to get up for the task unfortunately.

Although the new release I made still does not solve the problem, at least it brings GitHub actions, and super-easy-to-add tests, which should make the effort of using either type hints (much more widespread nowadays) or a second error code a breeze 🤞🏾

Anyone up for the task? 😄

@gforcada
Copy link
Owner

gforcada commented Oct 8, 2022

To illustrate, a test to verify that indeed, this is still reported as an error:

    def test_std_library(self):
        source = """
        def modulo(foo: int, bar: int) -> int:
            return foo % bar
        """
        self.check_code(source)

Add this to run_tests.py and then:

python3.10 -m venv venv
. venv/bin/activate
pip install -r requirements.txt
pytest run_tests.py

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

No branches or pull requests

3 participants