This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: update mkpkg to latest coding standards
Type: Stage: resolved
Components: Distutils2 Versions: Python 3.3, 3rd party
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: eric.araujo, meatballhat, michael.mulich, tarek, titus
Priority: normal Keywords: patch

Created on 2010-05-01 15:22 by meatballhat, last changed 2022-04-11 14:57 by admin. This issue is now closed.

File name Uploaded Description Edit
mkpkg-round-of-pylinting.patch meatballhat, 2010-05-19 17:54 primary patch
Messages (14)
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) * (Python committer) 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 script, a utility that helps writing 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 `` 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

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) * (Python committer) 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 ```` per instruction from jafo, am pushing to a branch 'mbh/mkpkg-cleanup' of my distutils2 fork:

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) * (Python committer) 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 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) * (Python committer) 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) * (Python committer) 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 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, 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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.
Date User Action Args
2022-04-11 14:57:00adminsetgithub: 52837
2014-03-13 03:59:55eric.araujosetstatus: open -> closed
resolution: accepted -> out of date
stage: needs patch -> resolved
2011-09-19 14:23:06eric.araujosetmessages: + msg144279
versions: + Python 3.3
2011-06-09 22:38:56michael.mulichsetnosy: + michael.mulich
2011-04-03 16:28:08eric.araujounlinkissue8253 dependencies
2011-04-03 16:27:14eric.araujosetmessages: + msg132857
2011-04-03 16:25:38eric.araujounlinkissue8252 dependencies
2011-01-23 01:41:38jafosetnosy: - jafo
2010-11-06 14:34:56eric.araujounlinkissue8679 dependencies
2010-09-30 01:54:54eric.araujolinkissue8679 dependencies
2010-09-30 01:18:32eric.araujosetversions: + 3rd party, - Python 2.6, Python 2.5, Python 3.1, Python 2.7, Python 3.2
2010-07-30 01:56:58eric.araujosetmessages: + msg112037
2010-07-30 01:31:06eric.araujosetmessages: + msg112036
2010-07-18 00:09:51eric.araujolinkissue8253 dependencies
2010-07-17 21:37:28eric.araujosetassignee: tarek -> eric.araujo
versions: - Python 3.3
nosy: + titus

messages: + msg110605
resolution: accepted
stage: needs patch
2010-07-17 21:30:48eric.araujolinkissue8252 dependencies
2010-06-02 00:40:03meatballhatsetmessages: + msg106870
2010-06-01 17:56:49eric.araujosetmessages: + msg106851
2010-05-19 17:55:01meatballhatsetfiles: + mkpkg-round-of-pylinting.patch
2010-05-19 17:54:43meatballhatsetfiles: - mkpkg-round-of-pylinting.patch
2010-05-19 17:49:01meatballhatsetfiles: - mkpkg-round-of-pylinting.patch
2010-05-19 17:48:49meatballhatsetfiles: + mkpkg-round-of-pylinting.patch
2010-05-19 17:22:24meatballhatsetfiles: + mkpkg-round-of-pylinting.patch
2010-05-19 17:21:23meatballhatsetfiles: - mkpkg-round-of-pylinting.patch
2010-05-18 02:57:47meatballhatsetmessages: + msg105947
2010-05-18 02:31:30meatballhatsetfiles: + mkpkg-round-of-pylinting.patch
keywords: + patch
2010-05-13 03:16:19meatballhatsetmessages: + msg105621
2010-05-06 23:44:31tareksetnosy: + jafo
messages: + msg105173
2010-05-06 23:33:22meatballhatsetmessages: + msg105172
2010-05-01 17:28:02meatballhatsetmessages: + msg104732
2010-05-01 15:34:26eric.araujosetversions: - 3rd party
nosy: + eric.araujo

messages: + msg104727

type: behavior ->
2010-05-01 15:22:10meatballhatcreate