Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

This is a preliminary review but I'm unsure whether it's a good solution.

Sure, thanks very much for it, nonetheless!

I would actually prefer not to do anything smarter as it was said on the issue. Using file descriptors instead seems a new feature not a bugfix. So I'm rather -1 on this one (there wasn't a real need for that and using file descriptors eventually complicates the logic)

I mildly disagree: there is a need for it, as demonstrated by the OP opening a ticket because the current implementation doesn't work for their purposes, while the new one would. Although admittedly probably not a very common one! I also don't think it complicates the logic very much, although I might design it slightly differently if I was starting from scratch (e.g. maybe have separate path and fd-based subclasses). I wanted to minimise changes, so didn't try anything like that in this patch.

It does completely change how the implementation works, and ofc there are ways the abstraction can leak (e.g. file descriptors are a global resource and holding an extra one open could in theory cause problems). I can add a way for the user to force using the original implementation, either globally and/or on a per-chdir object basis, if it would be helpful, but generally prefer to avoid that sort of thing unless necessary (we all prefer one way to do something, after all).

I think it is a useful change that is almost a strict improvement on the current implementation anywhere it is applicable, but I can certainly accept other people's opinions differing on this, and would understand if it is rejected.

Although admittedly probably not a very common one

And this is usually a good indicator of why it shouldn't be part of the standard library.

there is a need for it, as demonstrated by the OP opening a ticket because the current implementation doesn't work for their purposes

That's not sufficient IMO.

I can add a way for the user to force using the original implementatio

That's not how the standard library is meant to be used. There should preferrably one way to do something.

I also don't think it complicates the logic very much

I'd say it's up to the module's maintainer but I still think it does complicate it. In addition, it's not a universal solution as it is conditioned to the platform supporting it. So I'm not really fond of that as well.

Although admittedly probably not a very common one

And this is usually a good indicator of why it shouldn't be part of the standard library.

Quite possibly, but it is, so IMO we may as well make it work as best we can.

That's not how the standard library is meant to be used. There should preferrably one way to do something.

Yes, agreed, as I said.

I also don't think it complicates the logic very much

I'd say it's up to the module's maintainer but I still think it does complicate it. In addition, it's not a universal solution as it is conditioned to the platform supporting it. So I'm not really fond of that as well.

Ah, there are maintainers for this module: I should have checked the "experts index" and tagged them in to start with (ping @ncoghlan and @1st1).

With regards "conditioned to the platform": IMV this is all inherently platform/filesystem dependent, especially the conditions under which it succeeds/fails, just like os.chdir which it wraps. We can only do the best we can with what is available. This change should not change the semantics beyond making it work as expected in a few situations where it currently errors.

However, to repeat, I totally accept this is a significant implementation change and may not be justified. If the maintainers and/or other core devs don't like it then fair enough!

As some comments in the issue hinted at, holding on to the fd for the duration of the context manager could cause problems: not just using more of a scarce resource, but also potentially preventing operations on the old cwd. Scenarios where that would matter are surely not common, but this may be a compatibility issue for existing users.

As some comments in the issue hinted at, holding on to the fd for the duration of the context manager could cause problems: not just using more of a scarce resource, but also potentially preventing operations on the old cwd. Scenarios where that would matter are surely not common, but this may be a compatibility issue for existing users.

I don't think it should prevent any operations on Linux, but please let me know if I missed anything. It certainly might on Windows, but I was under the impression Windows doesn't support using file descriptors with chdir, and so this change wouldn't apply there anyway. I'm not sure about any other platforms we support.

However, an fd-based alternative could easily be provided on pypi, or just added in directly by those users who care, so it isn't a big deal if it remains as-is in the std library. If people think the risk and/or complexity isn't worth it I'll withdraw the PR. In that case the issue should be closed as unplanned, if I understand things correctly.

Thanks for the reviews and feedback, everyone!