On 12/08/2014 03:45 AM, Dylan Baker wrote: > Hi Chad, > > I have a few comments below for you, mostly relating to the use of the > global keyword.
I admit that I had trouble with the global variables in this patch. Python wasn't behaving like C, and that confused me. >> diff --git a/piglit.conf.example b/piglit.conf.example >> index a864c01..0ac0f4f 100644 >> --- a/piglit.conf.example >> +++ b/piglit.conf.example >> @@ -33,6 +33,13 @@ bindir=/home/usr/oclconform >> testA >> testB >> >> +;[deqp-gles3] >> +; >> +; Path to the deqp-gles3 executable. You can also set this with the >> environment >> +; variable PIGLIT_DEQP_GLES3_EXE. Piglit will run the dEQP-GLES3 tests if >> and >> +; only if this option is set. >> +;exe=/home/knuth/deqp/modules/gles3/deqp-gles3 > > nit: bin over exe? (feel free to ignore, not a strong opinion) 'bin' does feel more Linuxy than 'exe'. I'll change it. >> +import ConfigParser >> +import os >> +import shutil > > unused > >> +import subprocess >> +import sys > > unused > >> +import tempfile > > unused You found the breadcrumbs of the patch's evolution ;) I'll remove the unused imports. >> + >> +import framework.profile >> + >> + > > You can lose a newline here if you want > >> +__all__ = ['profile'] >> + >> + > > And here I thought the PEP8 tools complained if toplevel objects were not separated by two newlines... I'll squash the space as you suggest. >> +_deqp_gles3_exe = None >> + >> + >> +def get_option(env_varname, config_option): >> + """Query the given environment variable and then piglit.conf for the >> + option. Return None if the option is unset. >> + """ > > newline before the closing """ not after Ok. >> + >> + opt = os.environ.get(env_varname, None) >> + if opt is not None: >> + return opt >> + >> + try: >> + opt = framework.core.PIGLIT_CONFIG.get(config_option[0], >> + config_option[1]) >> + except ConfigParser.NoSectionError, ConfigParser.NoOptionError: >> + pass >> + >> + return opt >> + >> + >> +def get_deqp_gles3_exe(): >> + """Get path to the deqp-gles3 executable. Idempotent.""" >> + global _deqp_gles3_exe > > No global please. > > I'd use a pattern like this: > > ```python > def _set_deqp_gles3_exe(): > ... > > _DEQP_GLES3_EXE = _set_deqp_gles3_exe() > > [rest of module] > ``` > > Then you can simply rely on the constant at that point. Ok, I'll initialize the global constant near the top of file. By the way, does GMail properly syntax-highlight your email when you write "```python"? Or is that just a habit of yours? >> + assert(os.path.exists(caselist_path)) > > assert is a keyword not a function, it doesn't need parentheses. It's > also advisable not to use them, because assert + a tuple doesn't do what > you would expect. Good to know. Will fix. >> +class DEQPTest(framework.profile.Test): >> + >> + def __init__(self, case_name): >> + command = [ >> + get_deqp_gles3_exe(), >> + '--deqp-case=' + case_name, >> + '--deqp-visibility=hidden', >> + ] >> + framework.profile.Test.__init__(self, command) > > Please use super instead an explicit call to Test.__init__. Ok. >> +def add_tests(): >> + global profile > > eww... global. Please don't use global. > > I think you could solve this by moving the profile declaration to the > top of the module, or you could use a pattern like xts.py does, where > the populate_profile() function returns a framework.profile.TestProfile > object. Oh... you've solved my mystery: add_tests() believed 'profile' was a local variable because 'profile' was declared after add_tests(). I'll fix the patch to remove the global keywords and resubmit.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
