Re: Python and script header definitions for modules
Huidae Cho <[email protected]> Sun, 13 Jun 2021 11:02:28 -0400
| Newsgroups | gmane.comp.gis.grass.devel |
|---|---|
| Message-ID | <CANEOyhqCmZGJ8b2xzkyO_Zx8iGXHTh+jik5khyUstuO0BP8n=Q@mail.gmail.com> |
--===============9099632301277028218== Content-Type: multipart/alternative; boundary="00000000000030e3a505c4a7085f" --00000000000030e3a505c4a7085f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Vaclav, It's a great discussion. I *personally* think that enforcing Black or Flake8 is less ideal [1], but coding consistency is always good, I agree. Just one excerpt from [1]: "Autoformatters like Black are soulless, they won't understand how each specific case will be most readable. This should always be the developer's concern." Anyway, I consider it a necessary evil. Option 3 is readily available with no changes in the core library, which is great. It just takes mental effort to add the extra comment (# noqa: E501, especially this number, I'll keep forgetting it). Does it even solve no space between # and %, I hope? Like option 5 as well, but can we embed YAML code as Python comments, not as a separate file, which I don't like. Maybe, as option 6, we write a YAML file and create a small utility that translates it to the current parser format in option 3 and replace it in the Python script? Maybe, then, some developers just write header definitions manually without YAML at all and running the extra script is an additional burden. Option 4 may not be too bad. We can just concatenate any lines with more than a certain number (4?) of leading spaces to the current definition. My biggest complaint is "#-space-%-space-key:-space-value". I just have to type too many spaces manually. We could write a small script that handles this, but again, that's an extra effort. Is it possible to create hooks so we checkout "#%" and commit "#-space-%" automatically? Maybe, even long definitions can be handled in the same way? Best, Huidae [1] https://luminousmen.com/post/my-unpopular-opinion-about-black-code-formatte= r On Tue, Apr 20, 2021 at 11:11 PM Vaclav Petras <[email protected]> wrote= : > Dear all, > > After PR 1446, g.parser now accepts `# %` in addition to `#%` in order to > allow following of the Python practice starting a block comment with `# `= . > This is checked by Flake8 E265. This resolved numerous warnings which you > would otherwise get for each module when using Flake8 with default, that > is, expected, settings. Consequently, users writing their own > scripts/modules can use Flake8 with default settings without getting a > flood of messages. > > However, not all issues were addressed by that. Many modules generate > "E501 Line too long" warnings. Following default settings for the Black > formater, we are now using 88 characters as line limit. Option and flag > descriptions and the value of descriptions describing options* have often > more characters than that. > > Currently, the E501 warning is disabled for all files in scripts and > temporal directories and for selected `g.gui.*.py` files in gui/wxpython. > However, we want to have the warning enabled globally. Although, Black > takes care of most of the long lines, it does not touch some lines, namel= y > long strings, and thus we want each file to be checked for Flake8 E501 > compliance. > > I would like to disable the check in each file just for the specific bloc= k > of code, however, this is not possible because Flake8 does not allow > disabling for blocks of code. Requiring the lines to be shorter won't wor= k > either because the descriptions item needs to be long. This leaves us wit= h > the following options: > > 1. Use per-file ignores to disable the warning and keep adding the files > which need it. This makes the Flake8 configuration larger over time while > our goal is to make it smaller over time. It also leaves the warning > disabled for (other code in) these files. > > 2. Add inline Flake8 ignore comment to the offending line. This will make > the line little longer, but it would be a good solution for normal Python > code. However, in case of the script header, we would need to teach > g.parser to understand trailing comments inside the relevant fields so th= at > the Flake8 ignore comment does not leak into the user interface descripti= on. > > 3. According to the PEP 257 - Docstring Conventions document, the > 'docstring of a script (a stand-alone program) should be usable as its > "usage" message.' I don't think sticking something like our parser > instructions into the docstring was what the authors had in mind. > Additionally, it is not used like this either as far as I can tell. > However, it would solve our issue. Just adding `"""` before the definitio= n > and adding `""" # noqa: E501` after that disables the warning for the > definition. The nice bonus is that we comply with PEP 257 by providing > module docstring and by describing its interface there (in some way). The > docstring presence is checked by Pylint's C0111 "Missing ... docstring". > > 4. We modify the parser so that at least some of the items can have > multiple lines. However, the parser is currently quite line-oriented and > the cost-benefit ratio may be low. > > 5. We change the script header definition format to some existing format > that can break lines, i.e. allowing multi-line values. A clear candidate > for the format is YAML or rather its simpler subset. This would have > additional benefits of making the format a standard format. Which in turn > would be beneficial for other things, e.g., for easier learning of the > syntax. Combining this with option 3, we could drop the `# %` part to mak= e > the YAML more readily readable. > > Let me know what you think, what option you like, what you would add, and > what you can help with. > > Thank you, > Vaclav > > > > * Clearly, we should do something about the naming here, too! > > > Example for option 3: > > """ > # %module > # % description: Imports raster data into a GRASS raster map using GDAL > library and reprojects on the fly. > ... > # %option > # % key: resample > # % type: string > # % required: no > # % multiple: no > # % options: > nearest,bilinear,bicubic,lanczos,bilinear_f,bicubic_f,lanczos_f > # % description: Resampling method to use for reprojection > # % descriptions: nearest;nearest neighbor;bilinear;bilinear > interpolation;bicubic;bicubic interpolation;lanczos;lanczos > filter;bilinear_f;bilinear interpolation with fallback;bicubic_f;bicubic > interpolation with fallback;lanczos_f;lanczos filter with fallback > # % answer: nearest > # % guisection: Output > # %end > ... > # %rules > # % required: output,-e > # %end > """ # noqa: E501 > > Links: > > https://github.com/OSGeo/grass/pull/1446 > https://www.flake8rules.com/rules/E265.html > https://www.flake8rules.com/rules/E501.html > http://pylint-messages.wikidot.com/messages:c0111 > _______________________________________________ > grass-dev mailing list > [email protected] > https://lists.osgeo.org/mailman/listinfo/grass-dev > --=20 Huidae Cho, Ph.D., GISP, /hid=C9=9B t=CD=A1=C9=95o/, =EC=A1=B0=ED=9D=AC=EB= =8C=80, =E6=9B=BA=E5=96=9C=E5=A4=A7 GRASS GIS Developer https://idea.isnew.info/ --00000000000030e3a505c4a7085f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div>Vaclav,</div><div><br></div><div>It's a great dis= cussion. I *personally* think that enforcing Black or Flake8 is less ideal = [1], but coding consistency is always good, I agree. Just one excerpt from = [1]: "Autoformatters like Black are soulless, they won't understan= d how each=20 specific case will be most readable. This should always be the=20 developer's concern." Anyway, I consider it a necessary evil.</div= ><div><br></div><div>Option 3 is readily available with no changes in the c= ore library, which is great. It just takes mental effort to add the extra c= omment (# noqa: E501, especially this number, I'll keep forgetting it).= Does it even solve no space between # and %, I hope?</div><div><br></div><= div>Like option 5 as well, but can we embed YAML code as Python comments, n= ot as a separate file, which I don't like. Maybe, as option 6, we write= a YAML file and create a small utility that translates it to the current p= arser format in option 3 and replace it in the Python script? Maybe, then, = some developers just write header definitions manually without YAML at all = and running the extra script is an additional burden.<br></div><div><br></d= iv><div>Option 4 may not be too bad. We can just concatenate any lines with= more than a certain number (4?) of leading spaces to the current definitio= n.</div><div><br></div><div>My biggest complaint is "#-space-%-space-k= ey:-space-value". I just have to type too many spaces manually. We cou= ld write a small script that handles this, but again, that's an extra e= ffort. Is it possible to create hooks so we checkout "#%" and com= mit "#-space-%" automatically? Maybe, even long definitions can b= e handled in the same way?<br></div><div><br></div><div>Best,</div><div>Hui= dae</div><div></div><div><br></div><div>[1] <a href=3D"https://luminousmen.= com/post/my-unpopular-opinion-about-black-code-formatter">https://luminousm= en.com/post/my-unpopular-opinion-about-black-code-formatter</a></div></div>= <br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Tue= , Apr 20, 2021 at 11:11 PM Vaclav Petras <<a href=3D"mailto:wenzeslaus@g= mail.com">[email protected]</a>> wrote:<br></div><blockquote class=3D= "gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(2= 04,204,204);padding-left:1ex"><div dir=3D"ltr"><div>Dear all,</div><div><br= ></div><div>After PR 1446, g.parser now accepts `# %` in addition to `#%` i= n order to allow following of the Python practice starting a block comment = with `# `. This is checked by Flake8 E265. This resolved numerous warnings = which you would otherwise get for each module when using Flake8 with defaul= t, that is, expected, settings. Consequently, users writing their own scrip= ts/modules can use Flake8 with default settings without getting a flood of = messages.<br></div><div><br></div><div>However, not all issues were address= ed by that. Many modules generate "E501 Line too long" warnings. = Following default settings for the Black formater, we are now using 88 char= acters as line limit. Option and flag descriptions and the value of descrip= tions describing options* have often more characters than that.</div><div><= br></div><div>Currently, the E501 warning is disabled for all files in scri= pts and temporal directories and for selected `g.gui.*.py` files in gui/wxp= ython. However, we want to have the warning enabled globally. Although, Bla= ck takes care of most of the long lines, it does not touch some lines, name= ly long strings, and thus we want each file to be checked for Flake8 E501 c= ompliance.</div><div><br></div><div>I would like to disable the check in ea= ch file just for the specific block of code, however, this is not possible = because Flake8 does not allow disabling for blocks of code. Requiring the l= ines to be shorter won't work either because the descriptions item need= s to be long. This leaves us with the following options:</div><div><br></di= v><div>1. Use per-file ignores to disable the warning and keep adding the f= iles which need it. This makes the Flake8 configuration larger over time wh= ile our goal is to make it smaller over time. It also leaves the warning di= sabled for (other code in) these files.<br></div><div><br></div><div>2. Add= inline Flake8 ignore comment to the offending line. This will make the lin= e little longer, but it would be a good solution for normal Python code. Ho= wever, in case of the script header, we would need to teach g.parser to und= erstand trailing comments inside the relevant fields so that the Flake8 ign= ore comment does not leak into the user interface description.<br></div><di= v><br></div><div>3. According to the PEP 257 - Docstring Conventions docume= nt, the 'docstring of a script (a stand-alone program) should be usable= as its "usage" message.' I don't think sticking somethin= g like our parser instructions into the docstring was what the authors had = in mind. Additionally, it is not used like this either as far as I can tell= . However, it would solve our issue. Just adding `"""` befor= e the definition and adding `""" =C2=A0# noqa: E501` after t= hat disables the warning for the definition. The nice bonus is that we comp= ly with PEP 257 by providing module docstring and by describing its interfa= ce there (in some way). The docstring presence is checked by Pylint's C= 0111 "Missing ... docstring".<br></div><div><br></div><div>4. We = modify the parser so that at least some of the items can have multiple line= s. However, the parser is currently quite line-oriented and the cost-benefi= t ratio may be low.</div><div><br></div><div>5. We change the script header= definition format to some existing format that can break lines, i.e. allow= ing multi-line values. A clear candidate for the format is YAML or rather = its simpler subset. This would have additional benefits of making the forma= t a standard format. Which in turn would be beneficial for other things, e.= g., for easier learning of the syntax. Combining this with option 3, we cou= ld drop the `# %` part to make the YAML more readily readable.<br></div><di= v><br></div><div>Let me know what you think, what option you like, what you= would add, and what you can help with.<br></div><div><br></div><div>Thank = you,<br></div><div></div><div>Vaclav<br></div><div><br></div><div><br></div= ><div><br></div><div>* Clearly, we should do something about the naming her= e, too!<br></div><div></div><div><br></div><div><br></div><div>Example for = option 3:<br></div><div><br></div><div>"""<br># %module<br>#= % description: Imports raster data into a GRASS raster map using GDAL libr= ary and reprojects on the fly.<br>...</div><div># %option<br># % key: resam= ple<br># % type: string<br># % required: no<br># % multiple: no<br># % opti= ons: nearest,bilinear,bicubic,lanczos,bilinear_f,bicubic_f,lanczos_f<br># %= description: Resampling method to use for reprojection<br># % descriptions= : nearest;nearest neighbor;bilinear;bilinear interpolation;bicubic;bicubic = interpolation;lanczos;lanczos filter;bilinear_f;bilinear interpolation with= fallback;bicubic_f;bicubic interpolation with fallback;lanczos_f;lanczos f= ilter with fallback<br># % answer: nearest<br># % guisection: Output<br># %= end<br>...<br># %rules<br># % required: output,-e<br># %end<br>""= " =C2=A0# noqa: E501</div><div><br></div><div>Links:</div><div><br></d= iv><div><div></div><div><a href=3D"https://github.com/OSGeo/grass/pull/1446= " target=3D"_blank">https://github.com/OSGeo/grass/pull/1446</a></div><div>= <a href=3D"https://www.flake8rules.com/rules/E265.html" target=3D"_blank">h= ttps://www.flake8rules.com/rules/E265.html</a></div><div><a href=3D"https:/= /www.flake8rules.com/rules/E501.html" target=3D"_blank">https://www.flake8r= ules.com/rules/E501.html</a></div><div><a href=3D"http://pylint-messages.wi= kidot.com/messages:c0111" target=3D"_blank">http://pylint-messages.wikidot.= com/messages:c0111</a></div></div></div> _______________________________________________<br> grass-dev mailing list<br> <a href=3D"mailto:[email protected]" target=3D"_blank">grass-dev@li= sts.osgeo.org</a><br> <a href=3D"https://lists.osgeo.org/mailman/listinfo/grass-dev" rel=3D"noref= errer" target=3D"_blank">https://lists.osgeo.org/mailman/listinfo/grass-dev= </a><br> </blockquote></div><br clear=3D"all"><br>-- <br><div dir=3D"ltr" class=3D"g= mail_signature"><div dir=3D"ltr"><div><div dir=3D"ltr"><div><div dir=3D"ltr= "><div><div dir=3D"ltr"><div><div dir=3D"ltr"><span><div><div dir=3D"ltr">H= uidae Cho, Ph.D., GISP, /hid=C9=9B t=CD=A1=C9=95o/, =EC=A1=B0=ED=9D=AC=EB= =8C=80, =E6=9B=BA=E5=96=9C=E5=A4=A7</div><div dir=3D"ltr">GRASS GIS Develop= er</div><div><a href=3D"https://idea.isnew.info/" target=3D"_blank">https:/= /idea.isnew.info/</a><br></div></div></span></div></div></div></div></div><= /div></div></div></div></div> --00000000000030e3a505c4a7085f-- --===============9099632301277028218== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ grass-dev mailing list [email protected] https://lists.osgeo.org/mailman/listinfo/grass-dev --===============9099632301277028218==--