-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
tokenize.__all__ list is incomplete #71299
Comments
That's a child issue of bpo-23883, created to propose a patch fixing tokenize module's __all__ list. Changes in tests go farther: I've changed import from from tokenize import ... to import tokenize and adjusted all its usages accordingly. The module must be imported in order to test its __all__ list through test.support.check__all__ helper and just adding this single import would either force us to either do
I think going third way: with just "import tokenize" and changing its uses in the rest of tests result in celarer code, but of course I guess this may be too much for a single patch. |
The blacklist is too long. I think it would be better to use white list. |
I disagree:
|
|
I think it might be better to add a separate “import tokenize as tokenize_module”. Or just “import tokenize” inside the test function. IMO changing all the names adds too much churn with minimal benefit. Maybe also should restore the “from unittest import . . .” line. But keep an “import unittest” line somewhere so the __main__ bit at the bottom works. Perhaps it would be good to merge MiscTestCase into the existing TestMisc class. |
Assumption here is that implementation details shouldn't "look" public - they should have names starting with "_"; I think blacklisting names in these tests encourages good practice - if something "looks" public, either:
But ok, assuming we go with whitelisting and plain self.assertCountEqual:
Should I then do: import token
expected = token.__all__ + ["COMMENT", "NL", "ENCODING", "TokenInfo", "TokenError", "detect_encoding", "untokenize", "open", "tokenize"] ?
I wouldn't call it minimal, it has some positive impact on readability, see last line from The Zen of Python. :) Of course, final call is yours. Single import of unittest is such a small change I would rather keep it. I fully agree existing TestMisc class is a good place for this test, though. |
Changing the names to tokenize.<name> does solve the problem of the tokenize module versus the tokenize() fuction, so I can accept this way since you prefer it. So I think that just leaves what to do with the actual test case. I don’t think it matters too much, but I would lean toward ensuring the test fails if someone adds a new implementation detail without an underscore prefix. It is also good to be explicit that the ISTERMINAL() etc functions are special cases. On the other hand, neither the original patch nor Jacek’s proposal for “expected = token.__all__ + ...” would pick up the fact that the tok_name dictionary is another special case copied from the “token” module. (See also bpo-25324.) |
Original patch meets these requirements. I've updated it with moving the test__all__ method to TestMisc class as suggested (tokenize_all.v2.patch). I'm also attaching the alternative version (tokenize_all.v2.1.patch) that uses self.assertCountEqual instead of support.check__all__ and whitelisting as Serhiy suggested; this version of test doesn't meet the requirements above. Yes, neither one challenge the tok_name (bpo-25324) problem, I'm not really sure whether is should, though. I'll try to solve it with separate patch if I find some time. |
Thanks Jacek. I would prefer to go with v2.patch. |
Co-authored-by: Unit03
Looks fixed by #105907, thanks everyone! |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
The text was updated successfully, but these errors were encountered: