This issue tracker has been migrated to GitHub ,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2016年05月24日 20:28 by Unit03, last changed 2022年04月11日 14:58 by admin. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| configparser_all.patch | Unit03, 2016年05月24日 20:28 | review | ||
| configparser_all.v2.patch | Unit03, 2016年09月06日 07:40 | review | ||
| Messages (8) | |||
|---|---|---|---|
| msg266266 - (view) | Author: Jacek Kołodziej (Unit03) * | Date: 2016年05月24日 20:28 | |
That's a child issue of #23883, created to propose a patch fixing configparser module's __all__ list. |
|||
| msg266330 - (view) | Author: Martin Panter (martin.panter) * (Python committer) | Date: 2016年05月25日 11:26 | |
Looks good to me |
|||
| msg266391 - (view) | Author: Łukasz Langa (lukasz.langa) * (Python committer) | Date: 2016年05月25日 19:52 | |
The reason we specifically omitted Error was two-fold:
- the name "Error" is very generic and during a star-import might easily shadow some other class of the same name;
- Error is only a base class for exceptions raised by configparser and as such isn't part of the public API. You can see the same behavior in concurrent.futures for example. However, now I noticed configparser.Error is listed in the documentation so the assertion that "it's not public API" is effectively incorrect.
So I'm torn a little here. On the one hand, it's nice to add Error for completeness. On the other hand, is this change solving a real issue or just satisfying your inner librarian? The reason we have to ask ourselves this question is that this change bears a small risk of breaking user code that was working before. Take a look at this example:
```
from wave import *
from configparser import *
cfg = ConfigParser()
cfg.read('appconfig.ini')
try:
with Wave_read(cfg['samples']['sad_trombone']) as wav:
n = wav.getnframes()
frames = wav.readframes(n)
except Error as e:
print("Invalid sample:", e)
except KeyError as e:
print("Can't find {!r} in the config".format(str(e)))
else:
play_sound(frames)
```
Sure, it's bad code but the point is: it was working before and had a decent error handling strategy. With the change in __all__, it will just crash because wave.Error was never caught.
Is this likely to happen? I don't think so. Knowing my luck, will it happen to somebody? Yeah. So the question remains: why do we want Error in __all__ in the first place? Is it worth it?
|
|||
| msg266421 - (view) | Author: Martin Panter (martin.panter) * (Python committer) | Date: 2016年05月26日 09:53 | |
My personal opinion is to include all public APIs. Names that are omitted from __all__ may not come up in pydoc, and it is surprising when I use "import * " in the interactive interpreter to play with a module and there is something missing. To mitigate the risk of breaking code, I have been maintaining a list of the modules affected at <https://docs.python.org/3.6/whatsnew/3.6.html#changes-in-the-python-api>, which warns that extra symbols will be imported in 3.6. On the other hand, there are other cases where people wanted to exclude APIs from __all__; I pointed out two at <https://bugs.python.org/issue26632#msg266117>. |
|||
| msg274411 - (view) | Author: Jacek Kołodziej (Unit03) * | Date: 2016年09月05日 17:43 | |
Łukasz, Martin - I'm not sure how to proceed here, of course both ways out are reasonable. I'd be happy to provide (however small) patch for either one (adding Error to __all__ or just adding test for __all__). :) My inner librarian would of course either push the Error to fully become part of public API or change its name to non-public _Error or something, but I'd much rather rely on you in this regard. |
|||
| msg274454 - (view) | Author: Łukasz Langa (lukasz.langa) * (Python committer) | Date: 2016年09月05日 22:40 | |
I'm still not convinced this change is *useful*. The list of incompatibilities in Python 3.6 in whatsnew is going to be used retrospectively by people already affected by a production issue, googling for an explanation as to what went wrong. I like the idea of adding a unit test that clarifies the omission is deliberate at this point. |
|||
| msg274536 - (view) | Author: Jacek Kołodziej (Unit03) * | Date: 2016年09月06日 07:40 | |
That's completely fine for me. I'm attaching the patch that just adds test for __all__, then. :) |
|||
| msg275271 - (view) | Author: Roundup Robot (python-dev) (Python triager) | Date: 2016年09月09日 07:02 | |
New changeset 739528288996 by Martin Panter in branch 'default': Issue #27106: Add test for configparser.__all__ https://hg.python.org/cpython/rev/739528288996 |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022年04月11日 14:58:31 | admin | set | github: 71293 |
| 2016年09月09日 07:55:11 | martin.panter | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
| 2016年09月09日 07:02:56 | python-dev | set | nosy:
+ python-dev messages: + msg275271 |
| 2016年09月06日 07:40:03 | Unit03 | set | files:
+ configparser_all.v2.patch messages: + msg274536 |
| 2016年09月05日 22:40:55 | lukasz.langa | set | messages: + msg274454 |
| 2016年09月05日 17:43:37 | Unit03 | set | messages: + msg274411 |
| 2016年05月26日 09:53:09 | martin.panter | set | messages: + msg266421 |
| 2016年05月25日 19:52:56 | lukasz.langa | set | messages: + msg266391 |
| 2016年05月25日 13:16:53 | martin.panter | link | issue23883 dependencies |
| 2016年05月25日 11:26:10 | martin.panter | set | nosy:
+ martin.panter messages: + msg266330 stage: patch review |
| 2016年05月25日 05:52:22 | serhiy.storchaka | set | nosy:
+ lukasz.langa |
| 2016年05月24日 20:28:25 | Unit03 | create | |