-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Modify the new 3.10.6 version of platform.py to work with platform.rs #4135
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
Conversation
I now think |
Funny, I was thinking that too, earlier, and gave it a try. (It worked fine.) But I figured there was a reason/preference for not doing it that way in the earlier versions. Anyway, I'll go ahead with that. (This will also take care of those tests that failed, that I forgot to check first -- oops! I'm curious though how the pre-3.10.6 version got through with the same issue. There's a cache variable that was removed, that the test tries to clear.) BTW, there are those two TODO items in the prior version that short-circuited a couple things. Let me take a closer look at those to see if they're important. I'll submit the change after I check those. |
There could be an historical issue we couldn't do. The change made long time ago and we had a huge progress on compatibility. Maybe we just couldn't make the best decision for every time. |
I confirmed, the old TODO's weren't necessary. However... The test now fails because they're doing some "interesting" things in test_platform.py, setting sys.version and sys.platform to fake out the platform.* functions. We could probably support this in platform.rs (have it check sys.version and sys.platform). But what they're doing isn't really a documented feature, and the rust code will get pretty convoluted. But what do you think? Should I just modify the test to handle our rust overrides better? (That link you sent earlier about updating tests was very informative. Thanks.) (I have other ideas that would involve changes to platform.py. But since we're trying to minimize changes there...) |
Latest commit assumes that the simplified approach (just the import at the end) and an |
de3175e
to
e400c60
Compare
I am really sorry, but after reviewing the failing test, I think |
Understood, no problem. (Regarding other issues to work on, I was going to do one of the number protocol items when I get a chance later.) |
This addresses #4111
@youknowone This is the original change I had planned to do. See my 3rd comment here regarding your try/except comment. Let me know if you'd like me to do something like that instead. (Although I'm not sure how the import of _platform would ever fail without other things falling apart first.)