| msg104726 - (view) |
Author: Dan Buch (meatballhat) |
Date: 2010-05-01 15:22 |
| On first glance, `distutils2.mkpkg` does not reflect the latest and greatest in Python coding standards. I'd like to take a stab at PEP-(7|8)'ing the whole thing, although I know there are other issues open to add features to the module, so I don't want to cause unnecessary merge pains. Thoughts? |
|
|
| msg104727 - (view) |
Author: Éric Araujo (eric.araujo) *  |
Date: 2010-05-01 15:34 |
| Pepeightification is ok for things like whitespace that do not break compatibility (don’t waste time doing it manually though, we have automated tools that can be used to reindent the whole of Distutils2). However, renaming classes and functions has to be done with caution, if there’s code out there that depends on the module. Moreover, PEP 8 does not tell everything, e.g. other PEPs describe good practices for doctrings, and a lot of idioms are not covered in PEP 8. Distutils2 also has the constraint of having to stay compatible with Python 2.4. Personally, I also find that the name of the script is misleading. That said, I don’t know the status of mkpkg. If we aim at removing the setup.py script, a utility that helps writing setup.py scripts is doomed to be removed. Or it may be repurposed to write a setup.cfg file. |
|
|
| msg104732 - (view) |
Author: Dan Buch (meatballhat) |
Date: 2010-05-01 17:28 |
| I probably shouldn't have dropped the PEP8 bomb so much as stated that I feel the module could use some updating. It's my (very much potentially wrong) understanding that `distutils2.mkpkg` isn't considered library code so much as the guts of a script. The reason for my concern about `distutils2.mkpkg` goes something like this: - it is a new addition to the distutils toolkit - if any kind of walkthrough is to be written for using distutils, there's a good chance the `mkpkg` script will be mentioned - curious folks like myself may look at the source code for the script - said curious folks may either be new to Python or new to programming in general - I don't want newcomers getting the wrong idea about Python coding standards I should also mention that I have the same concern(s) about everything in the `Demo` tree of CPython :) Even if `setup.py` files aren't the eventual goal, if they're around for even another 2 years I think the effort is justified. </2cents> |
|
|
| msg105172 - (view) |
Author: Dan Buch (meatballhat) |
Date: 2010-05-06 23:33 |
| bump. Would it be more helpful if I were to submit a patch, too, or is doing so prior to guidance from the driver (Tarek) frowned upon? |
|
|
| msg105173 - (view) |
Author: Tarek Ziadé (tarek) * |
Date: 2010-05-06 23:44 |
| I am adding Sean (jafo) who wrote this module. He'll have the best answer :) |
|
|
| msg105621 - (view) |
Author: Dan Buch (meatballhat) |
Date: 2010-05-13 03:16 |
| I've started work on cleanup of ``mkpkg.py`` per instruction from jafo, am pushing to a branch 'mbh/mkpkg-cleanup' of my distutils2 fork: http://bitbucket.org/meatballhat/distutils2/changeset/be40174c59e2 I'll attach patch(es) when finished :) |
|
|
| msg105947 - (view) |
Author: Dan Buch (meatballhat) |
Date: 2010-05-18 02:57 |
| The attached mkpkg-round-of-pylinting.patch is known to cleanly apply to tarek's branch @ 541f90ef0636 |
|
|
| msg106851 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-06-01 17:56 |
| I’ve started to review your patch but I find it a bid unwieldy to read. Would you mind exporting changesets to patches? I tried to look at your repo, but didn’t know which branch to look at. So, thanks for giving us the occasion of thinking about the patch workflow. Using changesets seems more natural since we use Bitbucket, while OTOH using regular diffs can streamline the history and remove unnecessary details (e.g. I’m not sure we want branch names to clutter the history forever (see http://stevelosh.com/blog/2009/08/a-guide-to-branching-in-mercurial/ for more info)). Would it be too much of a hassle to provide links or a Mercurial bundle of a set of small changesets? Having different patches would help us separate the non-controversial stuff (whitespace) from the refactoring and improvements. That said, I can offer a few remarks on the patch. - Don’t put editor settings in the files. - Changing “if s in 'yn'” to “if s in ('y', 'n')” is not really an improvement. It’s not more readable to always use tuples or frozensets for membership testing; str has __contains__ for a reason :) - Not sure how your print_ indirection is helpful. - I don’t know how much reST we want in the docstrings: None, some constructs, Sphinx extensions? Don’t worry if there are no replies in a few days, people are pretty busy. I’ll ask about the meta-issues (workflow, reST) on the ML if Tarek doesn’t answer here. Cheers :) |
|
|
| msg106870 - (view) |
Author: Dan Buch (meatballhat) |
Date: 2010-06-02 00:40 |
| @merwok much thanks for the feedback. After seeing how you're working via bitbucket I've decided to create a fresh fork from tarek and recreate my patch in multiple changesets all within the default branch. I'll update the issue when finished. |
|
|
| msg110605 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-07-17 21:37 |
| I’m assigning this bug to myself as part of my work on #8252. Dan, I’ll sort the various fixes in your patch and replay them incrementally. I’ll be pleased to work with you if you have time (find me in #distutils or through email), if you don’t thanks for the starting point, I’ll manage the rest :) I also had an epiphany: Why do we add a script when we have a nice command framework? So when the script is cleaned up, I’ll turn it into a distutils2.command.init command module. |
|
|
| msg112036 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-07-30 01:31 |
| I have six changesets that make progressive improvements on cosmetic things, so that the code gets more readable. You can start with http://bitbucket.org/Merwok/distutils2-killsetup/changeset/5e2906cabeab and follow the parent links. The other changes you made include changing the config file (+1, except with a simpler “mkpkg” section name), touching up the input/print code, and improving the helper methods (e.g. using codecs.open, using a simple string template instead of a lot of file.write). I had an illumination: We shouldn’t code input/print things but use the cmd module. That will be my next move, then I’ll look at your other code improvements (they will be a bit hard to spot, since your diff basically rewrites every line so I can’t easily see the changes made inside one function, but I’ll manage). |
|
|
| msg112037 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2010-07-30 01:56 |
| After speaking with a Montreal-Python hacker about my use case and reading the PyMOTW page for cmd, I think that using raw_input and print is actually the way to go. We don’t want the user to enter commands, just to answer questions. |
|
|
| msg132857 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-04-03 16:27 |
| FYI, the mkcfg module has seen a lot of change since last summer, some of which have bad style. I still have this bug on my todo list to fix that. |
|
|
| msg144279 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-09-19 14:23 |
| > Changing “if s in 'yn'” to “if s in ('y', 'n')” is not really an > improvement. It’s not more readable to always use tuples or frozensets > for membership testing; str has __contains__ for a reason :) Let me eat my words: using “in 'yn'” matches '' and 'yn', which we don’t want. Changed in 10fd0d0895a1. |
|
|