classification
Title: Improve queens demo (use argparse and singular form)
Type: enhancement Stage: resolved
Components: Demos and Tools Versions: Python 3.7
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: eric.smith, louielu, paka, r.david.murray, rhettinger, terry.reedy
Priority: normal Keywords: easy

Created on 2017-04-10 13:06 by paka, last changed 2017-04-18 16:42 by rhettinger. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1084 closed paka, 2017-04-11 13:21
Messages (11)
msg291428 - (view) Author: Pavlo Kapyshin (paka) * Date: 2017-04-10 13:06
Currently Tools/demo/queens.py:

 - does manual sys.argv parsing
 - says “Found 1 solutions”

I propose to: 1) use argparse; 2) if q.nfound == 1, use “solution”. I you are ok with this, I’ll make a pull request.
msg291432 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2017-04-10 17:18
I think these are reasonable improvements. Also, move the initialization of "silent" in to __new__.
msg291436 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-04-10 17:58
I'm not sure about using argparse.  Currently the demo uses no imports.  I'm not strongly against, though.

Did you mean __init__ instead of __new__?  Also, its value could be made True and False instead of 0 and 1.  (Which tells you how old this demo is.)
msg291438 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2017-04-10 18:05
Oops, yes, __init__.

Plenty of the demos use imports. I think it would be clearer with argparse, but I also don't feel strongly about it.
msg291440 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-04-10 18:25
Yeah, I was wondering if part of the demo was to show something that can be done with no library support...but that probably isn't the case.
msg291698 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-04-15 04:22
I prefer use of argparse.  Patch *looks* good reading it, but I cannot test yet.
msg291801 - (view) Author: Louie Lu (louielu) * Date: 2017-04-17 16:36
I make some review at GitHub.

Do David and Terry suggest to also fix the problem about PEP8 coding style, since this demo have many places didn't fit PEP8.
msg291804 - (view) Author: Pavlo Kapyshin (paka) * Date: 2017-04-17 17:16
Louie Lu, thank you for the review.

Different quotes were already used in file, so someone should
decide which ones must be used: single or double.

Regarding the -n option, I tried to be backwards compatible. Again,
a decision is needed.

The only place where I had a choice to not do something related to
PEP 8 was adding a line between Queens class and main function.
I decided to mention that fact in pull request. In other places I
had to follow contribution guide (therefore add code that follows
style guide).
msg291814 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-04-17 23:34
Please focus on the task at hand and don't worry about PEP-8ing the existing code.
msg291819 - (view) Author: Louie Lu (louielu) * Date: 2017-04-18 02:58
Pavlo, you are right, making the argument have backward compatible is good. String quote I'll prefer `'` more than `"`.
msg291838 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-04-18 16:42
After more thought on the subject, I think the n-queens demo should be left as is.  The demo is principally about the class and not about being a command-line tool.  Also, it is reasonable and appropriate to use sys.argv directly for such a simple API.  How to do so is a legitimate example itself.  Not all uses of sys.argv need argparse.

In other words, I don't think the proposed changes makes anyone's life better and in the some ways makes the example less approachable for teaching purposes (my area of expertise).

The example works fine as-is.  Changing it seems like unnecessary code churn and already eating some of our time.

Thank you for the idea.  It never hurts to look around for ways to improve code.  Though I don't think there was a new win in this case, don't let that discourage you.  Please continue to look for improvements :-)
History
Date User Action Args
2017-04-18 16:42:16rhettingersetstatus: open -> closed
messages: + msg291838

assignee: rhettinger
resolution: rejected
stage: resolved
2017-04-18 02:58:32louielusetmessages: + msg291819
2017-04-17 23:34:02rhettingersetnosy: + rhettinger
messages: + msg291814
2017-04-17 17:16:15pakasetmessages: + msg291804
2017-04-17 16:36:32louielusetnosy: + louielu
messages: + msg291801
2017-04-15 04:22:57terry.reedysetnosy: + terry.reedy
messages: + msg291698
2017-04-11 13:21:56pakasetpull_requests: + pull_request1228
2017-04-10 18:25:49r.david.murraysetmessages: + msg291440
2017-04-10 18:05:47eric.smithsetmessages: + msg291438
2017-04-10 17:58:21r.david.murraysetnosy: + r.david.murray
messages: + msg291436
2017-04-10 17:18:28eric.smithsetkeywords: + easy
nosy: + eric.smith
messages: + msg291432

2017-04-10 13:06:37pakacreate