Quoting Robert Bragg (2017-03-01 07:57:57) [snip] > >> +import xml.etree.ElementTree as ET > > > > use cElementTree, and please import as "et" instead of "ET", ET suggests to > > me > > as a pythonista that this is a class not a module. (I know ElementTree is > > capitalized, but that's a legacy thing, modern python style is modules are > > lower > > case with _, classes are CamelCase) > > Using cElementTree sounds good. > > If I grep Mesa I see for 6 different uses of ElementTree with 5 out of > 6 importing as ET. Really I just copied the style/name from other > examples I found including the reference manual: > https://docs.python.org/2/library/xml.etree.elementtree.html and ET > does seem to be the common abbreviation used with ElementTree. I don't > really have a strong oppinion here though and can just make the > change.
It's not a big deal to switch from ET to et, python style has a changed a bit
over the years, before PEP8 was written python code was generally written with
Java style names, ElementTree is one of those older modules.
>
> >
> >> +import argparse
> >> +import sys
> >
> > Also, please sort the imports
>
> okey, can do; the python style guide says:
>
> "Imports should be grouped in the following order:
>
> 1. standard library imports
> 2. related third party imports
> 3. local application/library specific imports
>
> You should put a blank line between each group of imports. "
>
> I'm not actually sure if mesa's general style of alphanumerically
> sorting everything should override, but luckily my only third party
> import begins with x.
Yeah, generally you sort them by group
[snip]
> >> +def emit_fadd(tmp_id, args):
> >> + c("double tmp" + str(tmp_id) +" = " + args[1] + " + " + args[0] + ";")
> >
> > "double tmp {tmp_id} = {args[1]} + {ags[0]};".format(tmp_id=tmp_id,
> > args=args)
> >
> > This avoids the need to explicitly convert to str, and is much more
> > readable.
> > I'd really recommend taking this approach for the rest of the file too, as
> > it
> > will make things much more readable.
>
> hmm, that seems like quite a bit more typing with some variables being
> written out three times this way :-/
>
> maybe we could go with a variation like: "double tmp {0} = {1} +
> {2};".format(tmp_id, args[1], args[0])
Yup, that's fine too.
>
> I've updated the emit_XYZ funcs to use .format() which seems more
> readable but I've still left quite a few uses of + based concatenation
> in places that I was less sure that it was more readable to use
> .format(). There was also at least once case that was made awkard
> since the string contains an open brace so I left it with + based
> concatenation.
Okay, you can use '{{' and '}}' to get a literal '{' and '}' in a formatter
string, but I understand why you might not want to. I appreciate the use of
str.format where it makes sense.
The other thing you can do, and it only makes sense in some places, is something
like this:
'MyCFunction_' + args[0] + '(' + args[1] + ')'
'MyCFunction_{0[0]}({0[1]})'.format(args)
again, use your best judgment on what it makes sense to use str.format
[snip]
> >> +/* Accumulation buffer offsets... */
> >> +.gpu_time_offset = 0,
> >> +.a_offset = 1,
> >> +.b_offset = 46,
> >> +.c_offset = 54,
> >> +""")
> >
> > textwrap.dedent is your friend for readability here.
>
> yep, especially with this code further indented within a main() function
>
>
> Okey, I've got these changes made and working locally so I'll send out
> an update soon - thanks for the comments!
>
> Br,
> - Robert
Thanks for making changes for me!
Dylan
signature.asc
Description: signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
