Skip to content
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

Do not use scandir package #996

Closed
mtelka opened this issue Apr 8, 2024 · 16 comments
Closed

Do not use scandir package #996

mtelka opened this issue Apr 8, 2024 · 16 comments

Comments

@mtelka
Copy link

mtelka commented Apr 8, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

mtelka commented Apr 8, 2024

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

@mtelka
Copy link
Author

mtelka commented Apr 8, 2024

But, OTOH, the pathlib2 project seems to be stale: jazzband/pathlib2#89.

@mrbean-bremen
Copy link
Member

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
Copy link
Author

mtelka commented Apr 8, 2024

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
Copy link
Member

mrbean-bremen commented Apr 8, 2024

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
Copy link
Author

mtelka commented Apr 8, 2024

Why do you need scandir at all? I suggest to remove it completely. Even from places like:

import scandir

@mrbean-bremen
Copy link
Member

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
Copy link
Member

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
Copy link
Author

mtelka commented Apr 8, 2024

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
Copy link
Member

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
Copy link
Author

mtelka commented Apr 8, 2024

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
Copy link
Member

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
Copy link
Author

mtelka commented Apr 11, 2024

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

@mrbean-bremen
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants