Skip to content

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented May 16, 2025

Summary

I noticed in #18137 that we fail to resolve all imports for some ecosystem repositories because they don't use the "traditional" src or flat layouts. Instead, they use a src layout but they use the project name instead of src for the "src folder name".

An example is psycopg where all source files are in psycopg/psycopg/_adapters_map.py:279:14

This PR extends our heuristics for src.root to recognize if a project has a <project>/<project> folder (and no src folder) and, if so, includes it in the src.root.

The motivation for this change is to support more projects with zero configuration.

I don't have a good sense of how common this project layout is, but it seems worth supporting, given that multiple ecosystem projects use it. This also makes our mypy primer results significantely more useful :)

Test plan

See mypy primer results, added unit tests.

@MichaReiser MichaReiser force-pushed the micha/project-name branch from 069d2bb to 7701d68 Compare May 16, 2025 17:57
@MichaReiser MichaReiser added configuration Related to settings and configuration ty Multi-file analysis & type inference labels May 16, 2025
@MichaReiser
Copy link
Member Author

MichaReiser commented May 16, 2025

and mypy primer hangs here too :( I wonder if it is just because of the too large diff. Any suggestions on how I could try this out?

@github-actions
Copy link
Contributor

github-actions bot commented May 17, 2025

mypy_primer results

No ecosystem changes detected ✅

@MichaReiser MichaReiser marked this pull request as ready for review May 17, 2025 16:15
@MichaReiser MichaReiser force-pushed the micha/project-name branch from 6cfa747 to 74d5f25 Compare May 17, 2025 16:25
@MichaReiser
Copy link
Member Author

MichaReiser commented May 17, 2025

  • Found 797 diagnostics
  • Found 646 diagnostics

There are no other changes to mypy primer, so maybe it's not worth changing the default just for one project. I'd be interested to hear from people with more knowledge of the Python ecosystem to hear how common this project structure is.

@MichaReiser
Copy link
Member Author

I think we should fix this for this specific mypy primer project for now. We can consider changing our heuristics if this comes up more frequently.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 19, 2025

This proposed change makes a lot of sense to me. The src/ convention is encouraged by most common package managers, but it's a fairly recent convention. Until relatively recently, it was very common to have the directory structure that this PR adds support for, and it's still not uncommon for older projects.

I think we should also add tests/ to the src.root default, as discussed in astral-sh/ty#414 (comment)

@carljm
Copy link
Contributor

carljm commented May 19, 2025

I also wouldn't have any objection to resurrecting and landing this PR; it doesn't seem to have any downside.

I'm not sure it's necessary that we check that <project-name>/<project-name> exists, as this PR currently does? That is, it would be fine to do this simply if <project-name>/ exists.

@MichaReiser MichaReiser reopened this May 19, 2025
@carljm
Copy link
Contributor

carljm commented May 19, 2025

I'm not sure it's necessary that we check that <project-name>/<project-name> exists, as this PR currently does? That is, it would be fine to do this simply if <project-name>/ exists.

Never mind this, Alex pointed out that if just <project-name>/ exists, it might be a flat layout, and we probably shouldn't add the extra root in that case.

@MichaReiser
Copy link
Member Author

I think we should also add tests/ to the src.root default, as discussed in astral-sh/ty#414 (comment)

I'm up for this but I suggest doing this in a separate PR. While related, it needs to apply to all layouts.

@AlexWaygood
Copy link
Member

sounds good

@MichaReiser MichaReiser merged commit 55a410a into main May 19, 2025
69 checks passed
@MichaReiser MichaReiser deleted the micha/project-name branch May 19, 2025 16:11
dcreager added a commit that referenced this pull request May 19, 2025
…rals

* origin/main:
  [ty] Add hint that PEP 604 union syntax is only available in 3.10+ (#18192)
  Unify `Message` variants (#18051)
  [`airflow`] Update `AIR301` and `AIR311` with the latest Airflow implementations (#17985)
  [`airflow`] Move rules from `AIR312` to `AIR302` (#17940)
  [ty] Small LSP cleanups (#18201)
  [ty] Show related information in diagnostic (#17359)
  Default `src.root` to `['.', '<project_name>']` if the directory exists (#18141)
dcreager added a commit that referenced this pull request May 19, 2025
* main:
  [ty] Use first matching constructor overload when inferring specializations (#18204)
  [ty] Add hint that PEP 604 union syntax is only available in 3.10+ (#18192)
  Unify `Message` variants (#18051)
  [`airflow`] Update `AIR301` and `AIR311` with the latest Airflow implementations (#17985)
  [`airflow`] Move rules from `AIR312` to `AIR302` (#17940)
  [ty] Small LSP cleanups (#18201)
  [ty] Show related information in diagnostic (#17359)
  Default `src.root` to `['.', '<project_name>']` if the directory exists (#18141)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

configuration Related to settings and configuration ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants