-
Notifications
You must be signed in to change notification settings - Fork 4
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
Pathlib everywhere #125
Pathlib everywhere #125
Conversation
3f1cc2b
to
594d83c
Compare
Interestingly, on all python versions works but on 3.10 🤷🏾 |
The 3.10 error is caused by python/cpython#88227 |
594d83c
to
586dde5
Compare
3.10 is still a problem. Weird. Looks like the bug I mentioned. |
4129319
to
5d5301a
Compare
The file `src/Products/CMFCore/tests/fake_skins/fake_skin/test_text_latin1.txt` in `Products.CMFCore` breaks while trying to read it with a Unicode error. Rather than breaking, at least report about it.
5d5301a
to
2e364fd
Compare
Yes, we could do that indeed, maybe with a big disclaimer, or directly a |
Hm, I updated the PR from master as I wanted to get the update to the "setup-python" action merged. But it also grabbed some other changes from master. Anyway, I see it still fails on 3.10. |
I've removed 3.10 from the tests and added a note to the changelog. I assume most people using this tool use a more recent python version. And otherwise they'll probably look at the changelog when they see a sudden error. |
Coverage went down @gforcada, if you're happy with my mucking-about with the tests, you can merge it. |
While looking at the code I noticed that we were using plenty of
os.sep
andos.path.join
that since Python 3.4 is so much more nicer to usepathlib.Path
instead.The diff turned out quite big, but it's only about replacing old with new usages, no functionality changes.
As an extra bonus there are a couple of minor improvements added 😄