Remove most `from mymodule import *`
Closed this issue · 12 comments
As PEP8 says, wildcard imports should be avoided.
Wildcard imports (from import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools"
http://www.python.org/dev/peps/pep-0008/#imports
Currently, this happens 141 times inside this repository (grep -r "import \*" ./, without considering those in docs files).
./compilevisual.py:from visual import *
./compilevisual.py:from visual.graph import *
./compilevisual.py:from visual.controls import *
./compilevisual.py:from visual.filedialog import *
./compilevisual.py:from visual.factorial import *
./compilevisual.py:from vis import *
./compilevisual.py:from vis.graph import *
./compilevisual.py:from vis.controls import *
./compilevisual.py:from vis.filedialog import *
./compilevisual.py:from vis.factorial import *
./site-packages/visual/text.py:from visual import *
./site-packages/visual/filedialog.py:from visual_common.filedialog import *
./site-packages/visual/visual_all.py:from math import *
./site-packages/visual/visual_all.py:from numpy import *
./site-packages/visual/factorial.py:from visual_common.factorial import *
./site-packages/visual/__init__.py:from visual.visual_all import * # this statement not included in vis/__init__.py
./site-packages/visual/__init__.py:from visual_common.create_display import *
./site-packages/visual/examples/electric_motor.py:from visual import *
./site-packages/visual/examples/dipole.py:from visual import *
./site-packages/visual/examples/controlstest.py:from visual import *
./site-packages/visual/examples/controlstest.py:from visual.controls import *
./site-packages/visual/examples/wave.py:from visual import *
./site-packages/visual/examples/graphtest.py:from visual import *
./site-packages/visual/examples/graphtest.py:from visual.graph import *
./site-packages/visual/examples/texttest.py:from visual import *
./site-packages/visual/examples/texttest.py:from visual.text import *
./site-packages/visual/examples/hanoi.py:from visual import *
./site-packages/visual/examples/toroid.py:from visual import *
./site-packages/visual/examples/gyro2.py:from visual import *
./site-packages/visual/examples/gyro2.py:#from visual.graph import *
./site-packages/visual/examples/bounce2.py:from visual import *
./site-packages/visual/examples/crossproduct.py:from visual import *
./site-packages/visual/examples/faces_cone.py:from visual import *
./site-packages/visual/examples/medusa.py:from visual import *
./site-packages/visual/examples/medusa.py:from random import *
./site-packages/visual/examples/convex.py:from visual import *
./site-packages/visual/examples/randombox.py:from visual import *
./site-packages/visual/examples/labels.py:from visual import *
./site-packages/visual/examples/gyro.py:from visual import *
./site-packages/visual/examples/colorsliders.py:from visual import *
./site-packages/visual/examples/mandelbrot.py:from visual import *
./site-packages/visual/examples/mandelbrot.py:from visual.graph import *
./site-packages/visual/examples/extruded_columns.py:from visual import *
./site-packages/visual/examples/stonehenge.py:from visual import *
./site-packages/visual/examples/doublependulum.py:from visual import *
./site-packages/visual/examples/doublependulum.py:#from visual.graph import *
./site-packages/visual/examples/text3D.py:from visual import *
./site-packages/visual/examples/boxlighttest.py:from visual import *
./site-packages/visual/examples/conch.py:from visual import *
./site-packages/visual/examples/texturetest.py:from visual import *
./site-packages/visual/examples/lathe.py:from visual import *
./site-packages/visual/examples/tictacdat.py:from visual import *
./site-packages/visual/examples/toroid_drag.py:from visual import *
./site-packages/visual/examples/drape.py:from visual import *
./site-packages/visual/examples/extrusion_overview.py:from visual import *
./site-packages/visual/examples/eventHandlers.py:from visual import *
./site-packages/visual/examples/bounce.py:from visual import *
./site-packages/visual/examples/differential_gear.py:from visual import *
./site-packages/visual/examples/stars.py:from visual import *
./site-packages/visual/examples/widgets.py:from visual import *
./site-packages/visual/examples/crystal.py:from visual import *
./site-packages/visual/examples/keyinput.py:from visual import *
./site-packages/visual/examples/material_test.py:from visual import *
./site-packages/visual/examples/orbit.py:from visual import *
./site-packages/visual/examples/lorenz.py:from visual import *
./site-packages/visual/examples/gas.py:from visual import *
./site-packages/visual/examples/gas.py:from visual.graph import *
./site-packages/visual/examples/planar.py:from visual import *
./site-packages/visual/examples/texture_and_lighting.py:from visual import *
./site-packages/visual/examples/tictac.py:from visual import *
./site-packages/visual/examples/tictac.py:from tictacdat import *
./site-packages/visual/examples/faces_heightfield.py:from visual import *
./site-packages/visual/examples/extrusion_examples.py:from visual import *
./site-packages/visual/graph.py:from visual_common.graph import *
./site-packages/visual/controls.py:from visual_common.controls import *
./site-packages/visual_common/filedialog.py:from visual import *
./site-packages/vis/filedialog.py:from visual_common.filedialog import *
./site-packages/vis/factorial.py:from visual_common.factorial import *
./site-packages/vis/__init__.py:from visual_common.create_display import *
./site-packages/vis/graph.py:from visual_common.graph import *
./site-packages/vis/controls.py:from visual_common.controls import *
./site-packages/vidle2/configSectionNameDialog.py:from Tkinter import *
./site-packages/vidle2/SearchDialog.py:from Tkinter import *
./site-packages/vidle2/GrepDialog.py:from Tkinter import *
./site-packages/vidle2/CallTipWindow.py:from Tkinter import *
./site-packages/vidle2/IOBinding.py:from Tkinter import *
./site-packages/vidle2/AutoCompleteWindow.py:from Tkinter import *
./site-packages/vidle2/configDialog.py:from Tkinter import *
./site-packages/vidle2/ToolTip.py:from Tkinter import *
./site-packages/vidle2/SearchEngine.py:from Tkinter import *
./site-packages/vidle2/ScrolledList.py:from Tkinter import *
./site-packages/vidle2/configHelpSourceEdit.py:from Tkinter import *
./site-packages/vidle2/SearchDialogBase.py:from Tkinter import *
./site-packages/vidle2/TreeWidget.py:from Tkinter import *
./site-packages/vidle2/WindowList.py:from Tkinter import *
./site-packages/vidle2/tabbedpages.py:from Tkinter import *
./site-packages/vidle2/MultiStatusBar.py:from Tkinter import *
./site-packages/vidle2/EditorWindow.py:from Tkinter import *
./site-packages/vidle2/Debugger.py:from Tkinter import *
./site-packages/vidle2/OutputWindow.py:from Tkinter import *
./site-packages/vidle2/ColorDelegator.py:from Tkinter import *
./site-packages/vidle2/PyShell.py: from Tkinter import *
./site-packages/vidle2/FileList.py:from Tkinter import *
./site-packages/vidle2/WidgetRedirector.py:from Tkinter import *
./site-packages/vidle2/keybindingDialog.py:from Tkinter import *
./site-packages/vidle2/Percolator.py: from Tkinter import *
./site-packages/vidle2/ReplaceDialog.py:from Tkinter import *
./site-packages/vidle2/ObjectBrowser.py:from types import *
./site-packages/vidle2/aboutDialog.py:from Tkinter import *
./site-packages/vidle2/FileRevert.py:from Tkinter import *
./site-packages/vidle2/UndoDelegator.py:from Tkinter import *
./site-packages/vidle2/textView.py:from Tkinter import *
./site-packages/vidle3/configSectionNameDialog.py:from tkinter import *
./site-packages/vidle3/SearchDialog.py:from tkinter import *
./site-packages/vidle3/GrepDialog.py:from tkinter import *
./site-packages/vidle3/CallTipWindow.py:from tkinter import *
./site-packages/vidle3/IOBinding.py:from tkinter import *
./site-packages/vidle3/AutoCompleteWindow.py:from tkinter import *
./site-packages/vidle3/configDialog.py:from tkinter import *
./site-packages/vidle3/ToolTip.py:from tkinter import *
./site-packages/vidle3/SearchEngine.py:from tkinter import *
./site-packages/vidle3/ScrolledList.py:from tkinter import *
./site-packages/vidle3/configHelpSourceEdit.py:from tkinter import *
./site-packages/vidle3/SearchDialogBase.py:from tkinter import *
./site-packages/vidle3/TreeWidget.py:from tkinter import *
./site-packages/vidle3/WindowList.py:from tkinter import *
./site-packages/vidle3/tabbedpages.py:from tkinter import *
./site-packages/vidle3/MultiStatusBar.py:from tkinter import *
./site-packages/vidle3/EditorWindow.py:from tkinter import *
./site-packages/vidle3/Debugger.py:from tkinter import *
./site-packages/vidle3/OutputWindow.py:from tkinter import *
./site-packages/vidle3/ColorDelegator.py:from tkinter import *
./site-packages/vidle3/PyShell.py: from tkinter import *
./site-packages/vidle3/FileList.py:from tkinter import *
./site-packages/vidle3/WidgetRedirector.py:from tkinter import *
./site-packages/vidle3/keybindingDialog.py:from tkinter import *
./site-packages/vidle3/ReplaceDialog.py:from tkinter import *
./site-packages/vidle3/aboutDialog.py:from tkinter import *
./site-packages/vidle3/FileRevert.py:from tkinter import *
./site-packages/vidle3/UndoDelegator.py:from tkinter import *
./site-packages/vidle3/textView.py:from tkinter import *
It is well known that one should never import a module in the form "from visual import *", but this is a feature, not a bug, in the VPython environment. The first page of the full user documentation (http://vpython.org/contents/docs/index.html) explains to expert programmers what to use instead of "from visual import *".
The biggest number of VPython users, by far, is the many thousands of university engineering and science students who write short VPython programs every semester in the context of the Matter & Interactions physics curriculum. Very few of these students have ever written a computer program before, yet it is important in their physics education that they nevertheless write Python programs with 3D animations to learn the essence of computational modeling. Because the physics course is very crowded, it is not possible to teach more about programming (or computer science) than an extremely small subset of concepts: sequential execution, variable, assignment, while loop. Every addition to the set represents a major pedagogical cost, which is why we teach the use of "from visual import *". Not only that, but this import also imports numpy and the most important math functions, such as sin, cos, and sqrt. In other words, this is the Python motto "Batteries included" with a vengeance.
In the case of site-packages/visual/examples, these are all very short programs that can be of use to students who have been using "from visual import *". The user documentation also assumes this import, as is mentioned in the link given above.
In the case of the vidle2 and vidle3 files, the important thing to know is that VIDLE (for Python 2 or 3) is a modification of the official IDLE editor packages with the Pythons distributed at python.org; the details of why this fork are somewhat lengthy, but the novice users need these modifications. The many instances of "from tkinter import *" or "from Tkinter import *" are in the original IDLE code. I've been waiting several years for IDLE to catch up with VIDLE, and to be distributed with the standard Pythons, in which case VIDLE will be removed from the VPython repository. Happily, there has recently been major activity in the IDLE development community, so I'm cautiously optimistic that the logjam will be broken.
The program compilevisual.py is just used (on Windows) to make sure all binaries of .py files have been created, before building an installer.
The few other files that have "from visual import *" have them in order to make this statement work in user programs.
In short, there really isn't anything in the repository that needs fixing in this regard. What is there is all completely intentional. The criteria for goodness and badness are not the same as in most projects, because few projects are aimed at supporting novice programmers writing programs that generate navigable real-time 3D animations. Obviously any improvements to the underlying C++ and Python system code would be all to the good, but that's a different issue.
@BruceSherwood PEP8 does mention this:
Modules that are designed for use via from M import * should use the all mechanism to prevent exporting globals
http://www.python.org/dev/peps/pep-0008/#global-variable-names
That's fine for the public API (what users should access directly), so it's natural to use that in the examples. For the private parts of the code, I can't see any benefit of using wildcard imports. It will provoke object overwrite and create subtle, annoying bugs at some point, and makes the code much more difficult to understand.
For example, in visual/__init__.py
from visual.visual_all import *
from visual_common.create_display import *how am I supposed to know what am I getting from those imports?
There are just the following 7 files for which the issue is relevant, all of them in the "visual" file and unlikely to be used by experienced programmers:
./site-packages/visual/text.py:from visual import *
./site-packages/visual/filedialog.py:from visual_common.filedialog import *
./site-packages/visual/visual_all.py:from math import *
./site-packages/visual/visual_all.py:from numpy import *
./site-packages/visual/factorial.py:from visual_common.factorial import *
./site-packages/visual/init.py:from visual.visual_all import * # this statement not included in vis/init.py
./site-packages/visual/init.py:from visual_common.create_display import *
They have that form in order to make "from visual import *" work properly. The documentation for experienced programmers on the first page of the VPython documentation, which advises using the "vis" imports, is quite specific about what you get if you use "from visual import *":
As a convenience to novice programmers to provide everything that is needed to get started, the statement "from visual import *" imports all of the VPython features and executes "from math import *" and "from numpy import *". It also arranges that for routines common to both math and numpy such as sqrt, the much faster math routine is used when possible (when the argument is a scalar rather than an array).
.
They have that form in order to make "from visual import *" work properly
That is not correct
Also, as I mentioned previously, PEP8 is quite clear about this intended behavior:
Modules that are designed for use via from M import * should use the all mechanism to prevent exporting globals
This refers to __all__. If you don't know what that means, what it is for or else, check http://docs.python.org/2/tutorial/modules.html#importing-from-a-package
If you look at visual_all.py, it is importing all of math and numpy via from M import * mechanism. I'm sure the __all__ variable is not being used here because of the huge number of names this imports into the namespace. It would be impossible for vpython to always maintain this list in an up-to-date faction. In this case, exporting the globals from numpy and math is the desired effect, so using __all__ to prevent this would be counter productive.
@SethMMorton short answer: you are wrong
Long answer:
-
number of elements gotten this yields
from numpy import *? 576! (depends on numpy's version). That's insane -
set(dir(math)) & set(dir(numpy))tells us there are 31 name clashes. Very bad, wouldn't you say? -
Standards exist for a reason. Obviously there are exceptions but they need better arguments.
I've always been under the belief that if one doesn't suggest a viable alternative when discussing something they don't like, they are just complaining. What do you suggest to modify to add your desired features but still allow the current behavior in a backwards compatible way that does not obfuscate the code further? I understand that you want to define the __all__ variable, but can you show the proposed code that will achieve this? I think it will be more constructive to discus specific solutions.
There is indeed a major issue with backward compatibility. And I repeat that experienced programmers who are writing sizable programs can (and do) use "import vis" instead of "from visual import *", so there really isn't a problem to be solved. It's already been solved by the creation of the vis import scheme.
@SethMMorton It's not that I don't like something, it's that's it's wrong for the 3 reasons that I stated (number 2 & 3 are the most important).
I would propose an alternative if I knew what do you think are the really important things that must be imported (I guess they will be less than 500 :) ). The aforementioned name clashes must be taken into consideration.
@BruceSherwood Backwards compatibility isn't really a major issue if proper versioning is considered (i.e. major.minor.fix scheme) because it can be listed in the release notes in the section of "backwards incompatible changes" of the next major release (7.0 I think), as most software do.
And now that you mention it, the module/package naming (vis, visual, visual_common and visual_all) is certainly confusing.
To be honest, all these reason are why new projects are created based in existing ones, or just forked, as it happened with PIL and Pillow. If you read the mailing list archives when that discussion happened, you'll see many similarities with our emails and comments exchange in Github issues.
I don't mean to be rude or harsh (sometimes email "tone" can be deceiving), just give honest feedback.
In response to your first point, I don't use vpython in this manner so I cannot say what are the most important functions. It depends on each user's use case.
In response to your second point, take a look at site-packages/visual/visual_all.py. In this file the name clashes for the functions 'ceil','cos','cosh','exp','fabs','floor','fmod','frexp', 'ldexp','log','log10','modf','sin','sinh','sqrt', 'tan','tanh' are delt with. The other 14 could be dealt with in the same manner.
In response to your third point, I think Guido says it best in this PEP8 section, particularly these quotes: "But most importantly: know when to be inconsistent -- sometimes the style guide just doesn't apply." and "In particular: do not break backwards compatibility just to comply with this PEP!"
There is a very important issue that cannot easily be dealt with by the standard major release mechanism. At least in the physics education context, and maybe in other domains for all I know, there is a significant corpus of demo programs written in VPython that are used by faculty in their lectures, to vivify concepts. Often these programs are used in lecture halls whose Python/VPython installation is under the control of IT, not the faculty. The backward compatibility issue takes on dimensions that are difficult to cope with. The challenging move from VPython 5 to VPython 6, forced on us by the threat of a Carbon-based VPython not being usable on the Mac, did break some existing programs due to the necessity of have a rate statement in animation loops, but (1) most animation loops did already contain rate statements and (2) programs with rate statements run on both VPython 5 and VPython 6. It is difficult to see how one would eliminate "from visual import *" and not break many programs that faculty once or twice removed from coding issues depend on.
I appreciate the desire for cleanliness and following good software design criteria, and perhaps one could improve the VPython code that deals with imports. But the functionality of "from visual import *" must be preserved as is. And I repeat (yet again) that any programmer who wishes to avoid conflicts can use "import vis", so the only real issue is the pure Python part of the VPython code, which may be inelegant but has not been a source of problems. There is a long list of real problems to be addressed, as can be seen in this GitHub issues list. It seems to me a needless distraction to worry about import issues that affect no one when there are real issues that do affect people. (I would be working on them myself were it not that I'm facing a major deadline in a textbook revision.)
I like Guido's quote.
I could argue why some changes are bug fixes rather than style-revamps (thus the remark about Guido's warning doesn't not apply). Or why clean APIs are the most important thing in a software library.
But I give up. In think my time will be better invested in creating new tools rather than digging through old bad code, patching projects and bringing them up to date to current best practices. I do understand concerns about not wanting to break too many things and keep backwards compatibility. But that is a huge burden that prevents progress. And, in this stage of my life, I care about that, and creating, and building things that I can be proud of.
I hope some of my contributions are of some value to the project, like setting up Travis CI and fixing the installer as well as pointing out and insisting how crucial is to have docs and a test suite for projects mean to be used by more than a few people.
If any of the above comments are too harsh, I'm sorry, I didn't mean to offend everybody. I wish you and the project lots of success.