On Monday, December 08, 2014 07:42:18 AM Chad Versace wrote: > 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. >
two lines between toplevel functions and classes, one between imports and constants. Since you use vim, may I suggest Syntastic + pylint? This plus the missing imports was generated by that tool. > > > >> +_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? I was just looking for a good way to differentiate python code from the rest of the comment. Although, since I use gvim to write emails I'm pretty sure I could code something up... > > > > >> + 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: This is a digitally signed message part.
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
