[pantsng] Pythonic access to the rust source partition logic.#23160
[pantsng] Pythonic access to the rust source partition logic.#23160benjyw merged 3 commits intopantsbuild:mainfrom
Conversation
benjyw
changed the title
benjyw
added
the
release-notes:not-required
|
AI disclosure: I asked Claude to generate the unit tests. It did an OK job, but I did substantial manual editing to make the tests more concise and readable. |
| return tuple(str(path) for path in self.paths) | ||
|
|
||
| def commondir(self) -> str: | ||
| ret = os.path.commonpath(self.paths) |
There was a problem hiding this comment.
This can throw in a couple of circumstances. I think the only relevant one here is empty tuple - what’s the downstream effect?
I don’t think we’re worried about abs vs relative paths, nor about paths on different drives I’m guessing? Unless someone has some mount shenanigans
There was a problem hiding this comment.
I'll change to handle empty tuple, might as well be robust.
All these paths are expected to be relative to the repo root.
| @@ -0,0 +1,122 @@ | |||
| # Copyright 2025 Pants project contributors (see CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
I started writing it in 2025, so I think that is the applicable date? TBH dates in copyright notices are mostly meaningless. Copyright notices themselves are mostly meaningless...
| def path_strs(self) -> tuple[str, ...]: | ||
| return tuple(str(path) for path in self.paths) | ||
|
|
||
| def commondir(self) -> str: |
There was a problem hiding this comment.
- Maybe
common_dirinstead? I'm not a fan of running words together in identifiers. - Can this value be precomputed or just refer to
source_root?
There was a problem hiding this comment.
I was copying commonpath, but yeah, can change it.
It can be precomputed, but probably better to let it evaluate lazily, so I've made it a @memoized_method.
|
|
||
| def commondir(self) -> str: | ||
| ret = os.path.commonpath(self.paths) | ||
| if os.path.isfile(ret): |
There was a problem hiding this comment.
Is filesystem access here okay? Shouldn't this be found with PathMetadataRequest?
There was a problem hiding this comment.
Good catch, this is a little sloppy (even if probably fine in practice since the race condition of same path being a file and then a dir would be weird). I think I can actually get rid of this check entirely, since we expect the paths to be files, and the only way the commonpath can be a file is if the input args were a single file.
There was a problem hiding this comment.
Hmm, no, I can imagine a backend for which "sources" are directories, and I wouldn't want this to break. Will replace it with a rule.
There was a problem hiding this comment.
And I'd recommend that rule use path_metadata_request so that the rule invalidates correctly on changes if it is examining metadata.
There was a problem hiding this comment.
Yep, that is what I meant by switching it to a rule (a classmethod can't await path_metadata_request)
There was a problem hiding this comment.
OK, done and the test rewritten to accommodate.
|
PTAL, thanks! |
| # The args were a single file, so the commonpath is that file. We want its enclosing dir. | ||
| common_dir = os.path.dirname(commonpath) | ||
| else: | ||
| common_dir = commonpath |
There was a problem hiding this comment.
PathMetadataKind.SYMLINK can also be true here.
There was a problem hiding this comment.
Not sure what to do in that case. We would need to know if it's a symlink to a file or to a directory.
I guess we can chase symlinks? I will leave it as a TODO for now.
There was a problem hiding this comment.
Eh, it's easy to chase symlinks since we have symlink_target right there, so I will do it and add a test.
3 participants
Provides convenient Pythonic access to a partition
of source files and the config that applies to them.
Partitions never cross multiple source roots, even if
a single config applies to all source roots.