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

mypy.ini parsing trailing comma causes duplicate module error #11171

Open
amin-nejad opened this issue Sep 22, 2021 · 9 comments · May be fixed by #18472
Open

mypy.ini parsing trailing comma causes duplicate module error #11171

amin-nejad opened this issue Sep 22, 2021 · 9 comments · May be fixed by #18472
Labels
bug mypy got something wrong good-first-issue topic-configuration Configuration files and flags

Comments

@amin-nejad
Copy link

amin-nejad commented Sep 22, 2021

Documentation

I've opted for documentation but this could just as well have been a bug report or a feature request. The issue is with a trailing comma after the last file in a list of files in mypy.ini. e.g.:

[mypy]
files =
    myproject/package1,
    myproject/package2,
    myproject/module1.py,
    myproject/module2.py,

This results in a duplicate module error as follows:

myproject/package1/__init__.py: error: Duplicate module named "myproject.package1" (also at "myproject/package1/__init__.py")
myproject/package1/__init__.py: note: Are you missing an __init__.py? Alternatively, consider using --exclude to avoid checking one of them.
Found 1 error in 1 file (errors prevented further checking)

It appears to somehow read the first element in the list again. Both packages already have __init__.py files. The problem is that this error isn't very informative and it took me a while to realise this was the cause.

Possible solutions:

  • Document this here
  • Detect trailing comma and flag this to user instead of parsing the config file and returning a cryptic error
  • Ignore trailing commas for the final element of a list during parsing of the config file

mypy==0.910

@sumit-158
Copy link

can I work on this?

@amin-nejad
Copy link
Author

can I work on this?

No objections from me 🙂

@webknjaz
Copy link

webknjaz commented Jan 1, 2025

Just hit this as well. I think the right solution would be to allow trailing commas. It's a bit more obvious with TOML and shouldn't really differ in ini, UX-wise.

I haven't checked, but my hunch is that configparser might be reading this entire value as a giant string, like file1,file2,file3,, splitting it by comma. And then, the last element is an empty string. This translates to just calling MyPy in terminal and passing an empty string on CLI.

I have confirmed that passing that empty string as another file breaks in exactly the same way:

$ MYPY_PATH=src/ mypy --explicit-package-bases myproject/package1 myproject/package2 myproject/module1.py myproject/module2.py ''

Additionally, I've found that you can call it with a double empty string:

$ MYPY_PATH=src/ mypy --explicit-package-bases '' ''

and it would explode the same...

Interestingly, a single empty string argument seems fine:

$ MYPY_PATH=src/ mypy --explicit-package-bases ''

So it seems to me that there are 2 bugs here:

  1. MyPy's argument processing breaks when there's more than one file path argument passed and at least one of those paths is an empty string — this deserves a clear error output, regardless of whether it's coming from a config or a CLI arg
  2. The configparser-based reader should normalize the INI format to allow for trailing commas and disallow empty strings so that it behaves the same as TOML from the user perspective.

@hauntsaninja could you please change the label from documentation to bug? Also, this is probably a good-first-issue.

@webknjaz
Copy link

webknjaz commented Jan 1, 2025

It just occurred to me that the config parsing bug may affect other settings that are being interpreted as lists. Perhaps, that's worth checking by whoever gets to work on this.

@hauntsaninja hauntsaninja added bug mypy got something wrong good-first-issue and removed documentation labels Jan 1, 2025
@x612skm
Copy link
Contributor

x612skm commented Jan 2, 2025

Hi!, Can I take this up?

@webknjaz
Copy link

webknjaz commented Jan 3, 2025

@hauntsaninja thanks for the labels! Could you assign the issue to me and @x612skm? I'll help him navigate the process.

@brianschubert brianschubert added the topic-configuration Configuration files and flags label Jan 8, 2025
@brianschubert
Copy link
Collaborator

@x612skm @webknjaz Feel free to work on this! (we generally don't assign issues)

You can check out CONTRIBUTING.md and the Developer Guides for contributing guidelines and tips on where to start. Thanks!

@x612skm
Copy link
Contributor

x612skm commented Jan 10, 2025

Just for the fyi - I've started brainstorming into this and soon will be hitting a PR after a discussion with @webknjaz . Thanks!

@webknjaz
Copy link

webknjaz commented Jan 17, 2025

@brianschubert I've got some feedback for you.

We've found a weakness in the contributing docs. I asked Soubhik to write some tests and since it was a new module, the first idea was to start with something simple, like a test function. It took some time to notice that it wasn't being picked up by pytest. This was when I asked to push the change w/o a fix to the PR (#18472) to make sure it's failing, and it didn't.

I was able to find the respective pytest config that disables the default test collection, but I don't believe that a less experienced person would know where to look. Still, it was quite frustrating to troubleshoot.

I checked the contrib and other docs — there seems to be no mention of needing to use classes in test modules.

So the action item is to document this somewhere, the closer to the section on invoking the tests the better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong good-first-issue topic-configuration Configuration files and flags
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants