Move common utility functions to a common library #12773
Conversation
| @@ -162,7 +162,7 @@ extra-standard-library = [ | |||
| "pyexpat", | |||
| "zoneinfo", | |||
| ] | |||
| known-first-party = ["_metadata", "_utils"] | |||
| known-first-party = ["ts_utils", "_utils"] | |||
There was a problem hiding this comment.
No, there's still a scripts/sync_protobuf/_utils.py file.
There was a problem hiding this comment.
tbf I only named it _utils to reflect this config, and as it is right know, it wouldn't even affect the order as those scripts are only using stdlib.
I also see no issue moving or renaming scripts/sync_protobuf/_utils.py if that's wanted.
tests/mypy_test.py
Outdated
| @@ -22,8 +22,11 @@ | |||
| import tomli | |||
| from packaging.requirements import Requirement | |||
|
|
|||
| from _metadata import PackageDependencies, get_recursive_requirements, read_metadata | |||
| from _utils import ( | |||
| sys.path += ["lib"] | |||
There was a problem hiding this comment.
This is pretty unfortunate, and also seems to create a dependency on the working directory from which the code is invoked. Could we instead add pyproject.toml to the lib directory to make it installable, then add it as a local dependency to requirements.txt?
There was a problem hiding this comment.
Another option is to use Path(__file__).parent.parent / "lib". On the other hand, most scripts already assume that they're called from the root typeshed directory, as they are using Path("stubs") and similar constructs.
I'm open to all three solutions (pyproject.toml, just "lib", using Path(__file__)).
There was a problem hiding this comment.
Personally I'd rather scripts be agnostics of their working directory when possible.
Jelle's suggestions sounds clean to me and reflects what I'm used to do and see in monorepo JS/TS projects with a shared library (using npm workspaces). If you're actively modifying lib/ts_utils, you could editable install it.
|
Third-party test failure is unrelated (gdb). |
Prerequisite for #12772
Unfortunately, this is a bit hacky, but I don't have a better idea.