-
Notifications
You must be signed in to change notification settings - Fork 148
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
Path.relative_to() walk_up support #655
Path.relative_to() walk_up support #655
Conversation
36e7651
to
fe9c7d4
Compare
14d0348
to
4c70f56
Compare
What's all the socket stuff, and why did you reformat |
4c70f56
to
27e38b1
Compare
Apologies, I realized I messed up my change to split the socket test modifications out into a separate commit. That's now fixed, so you can see just the changes that were made to that test file |
f42ac6d
to
855a0c2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Could you elaborate on where this "standard" YAML formatting comes from? As far as I can tell, my formatting is consistent with the YAML 1.2 standard. |
This comment was marked as resolved.
This comment was marked as resolved.
Hi, I just wanted to add that I think it'd be good to open a separate issue/PR for changes unrelated to the title of this PR. I think the original changes are great and I think it's a lot less cognitive load on reviewers and agronholm to review when done separately. Cheers! |
The only reason I haven't is because they're indirectly related; the full test suite (which includes new tests relevant to this PR that were specifically requested) won't pass without the other test changes. At least locally. For some inexplicable reason it doesn't break on the macOS CI instances, so maybe that's good enough? I'm happy to do so though. I'll have to wrap my head around making this PR contingent on the PR you're requesting be added (it'll need to be merged first), but, other than that, I think it's fine Sorry for the confusion. From the beginning I've been indicating that I was unsure of how to handle the other changes w.r.t. this PR so I'm glad someone has weighed in. Thanks! |
855a0c2
to
1ac5988
Compare
This comment was marked as resolved.
This comment was marked as resolved.
The changes to args: [src, tests]
pass_filenames: false |
1ac5988
to
ce5d6f5
Compare
@uSpike - PR's have been split into #659, #660 and #661, with 659's pre-commit changes determined as irrelevant and removed. @agronholm - Thanks for your patience in all of this, and apologies for over-complicating things. I've completely isolated the changes into separate PRs so you can consider them independently, and made sure that each one is as minimal a change as possible. Thank you for the workaround. I've closed #659 accordingly. Please note that this PR will now fail to run the full test suite on macOS >= 13. Running |
dfeb9e9
to
50e18d5
Compare
50e18d5
to
739d682
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Adding support for the
walk_up
keyword argument introduced inPath.relative_to()
in python 3.12