pytest-dev / pyfakefs

Provides a fake file system that mocks the Python file system modules.
https://pytest-pyfakefs.readthedocs.io
Apache License 2.0
627 stars 88 forks source link

Do not use scandir package #996

Closed mtelka closed 1 month ago

mtelka commented 2 months ago

The scandir standalone package was needed for Python < 3.5 only. Since pyfakefs requires Python >= 3.7 the scandir should be no longer needed. Unfortunately, there are still some trace of scandir in the sources, for example in extra_requirements.txt or in .github/workflows/testsuite.yml.

mrbean-bremen commented 2 months ago

The same is true for pathlib2, which I didn't remove because it still was used in some installations despite that it is no longer needed. But I agree that it is time to remove these.

mrbean-bremen commented 2 months ago

Though checking the respective projects, it looks like they are both still updated for newer Python versions, meaning that they are probably still installed (and used) in some installations (given previous experience, in quite a lot). Removing the support would break these installations, so I'm thinking about deprecating it instead, and removing it in the next major version.

mtelka commented 2 months ago

I think the pathlib2 module is still needed for Python < 3.10.

mtelka commented 2 months ago

But, OTOH, the pathlib2 project seems to be stale: https://github.com/jazzband/pathlib2/issues/89.

mrbean-bremen commented 2 months ago

Just to clarify: are you using or packaging extra_requirements.txt? What is your concrete problem? Would importing theese libraries conditionally help for the time being?

mtelka commented 2 months ago

I package pyfakefs for OpenIndiana. I do not have any significant problem with it. I patched out scandir from extra_requirements.txt because it is not needed to run tests and we no longer have it packaged for OpenIndiana (because it is not needed for supported Python versions). The extra_requirements.txt file is used by tox for testing, so yes, I do use it.

I filed the issue here just to get things improved/cleaned.

mrbean-bremen commented 2 months ago

Ah ok, thanks. ~So if we would import them only if some environment variable is set (which we would set in the CI, as long as we want to retain these tests), this would help you, e.g. you wouldn't need to patch it out, right?~

Edit: I got this wrong. We already import them only if they exist, we just could remove them from the extra_requirement.txt and either put them into a separate requirements file (not used in tox.ini), or just manually install them in the CI.

mtelka commented 2 months ago

Why do you need scandir at all? I suggest to remove it completely. Even from places like: https://github.com/pytest-dev/pyfakefs/blob/f7c00ded4ccbf9cd9947bf52ea739935ddf7c983/pyfakefs/extra_packages.py#L23

mrbean-bremen commented 2 months ago

As I wrote, if people still use it, and I would remove it, it would no longer be patched. I realize that nobody actually needs it nowadays, but that does not mean that it is not used (e.g. imported in the code). I would indeed remove the patching, but only after some warning/deprecation period.

mrbean-bremen commented 2 months ago

Actually I think I need to sleep over this... Maybe you are right. This is another case than pathlib2, and maybe it could indeed be removed.

mtelka commented 2 months ago

Actually I think I need to sleep over this... Maybe you are right. This is another case than pathlib2, and maybe it could indeed be removed.

This depends. If you just use scandir then it could/should be removed, but if you do some nice/nasty hacks when it is used by 3rd party software to modify its behavior then the warning/deprecation is the best thing to do for now, I think. And, feel free to keep it in extra_requirements.txt until you are ready to completely remove it. Patching it out here is probably better than doing some hacks at your side to test it properly.

mrbean-bremen commented 2 months ago

but if you do some nice/nasty hacks when it is used by 3rd party software to modify its behavior

Well, what I basically do is checking if it is installed by trying to import it, and if it is installed, patch it the same way the built-in scandir is patched. So I think about putting both scandir and pathlib2 into some legacy_requirements.txt and use this additionally in the CI only - not a big deal. And raise deprecating warnings in case these are actually installed. If that is ok with you, I would go tis way (after handling a regression that also came up today, which may take some time - but I guess there is no rush here).

mtelka commented 2 months ago

Yes, this would be perfect for me. BTW, I do not need new release with this fixed/changed only. But since you have some regression then you are apparently going to release new version anyway soon...

Thank you.

mrbean-bremen commented 2 months ago

The simple approach I tried did not work as I wanted it to, so I need a bit more time for this - will have another go probably at the weekend. I decided to go ahead with the patch release to get the fix for the regression out, so this one did not get in, sorry.

mtelka commented 2 months ago

Not a problem at all. Take your time. This is not a high priority issue.

mrbean-bremen commented 1 month ago

Version with deprecation is released, usage will be removed in 6.0. Closing.