Add 'quiet' option to suppress screen output during %prun calls, edited dochelp

[andrewgiessel]

Edited magic.py to add an extra option to magic_prun(). Specifically, added -q to suppress output to the screen via the pager. This eases use of %prun to do profiling of multiple functions inside of another script.

[minrk]

This looks pretty good to me, though I would prefer it without the extra whitespace changes.

[takluyver]

The has_key method is deprecated - we should use if 'q' not in opts instead.

[andrewgiessel]

Changing it to if not 'q' in opts is fine by me- i initially coded it like
this, and then noticed that the 'r' option is parsed like this, so I
changed it to match. We should probably change both.

I can double check and look for extraneous whitespace.

What is the best way to proceed? (I'm new to this- first contrib!) Do I
recommit and then do another pull request?

cheers,

ag

On Tue, Dec 27, 2011 at 07:23, Thomas <
reply@reply.github.com

wrote:

@@ -1438,7 +1440,8 @@ def magic_prun(self, parameter_s ='',user_mode=1,
output = stdout_trap.getvalue()
output = output.rstrip()

•    page.page(output)
  

•    if not opts.has_key('q'):
  

The has_key method is deprecated - we should use if 'q' not in opts
instead.

---

Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1210/files#r315482

Andrew Giessel, PhD

Department of Neurobiology, Harvard Medical School
220 Longwood Ave Boston, MA 02115
ph: 617.432.7971 email: andrew_giessel@hms.harvard.edu

[takluyver]

Yes, some of the older code was written like that, and hasn't been updated. We're slowly updating it as we revisit it.

[takluyver]

Hmm, missed your question. Just add another commit after this one, and push it to the same branch (we usually make a feature branch, rather than working on master). The pull request automatically updates when you add commits.

[minrk]

You don't need to make a new PR - any changes to the branch associated with this PR (master) will be reflected here. You can even remove/edit commits and replace them with a force-push (push -f).

[andrewgiessel]

There are a lot of places in this file where if dict.has_key(key) is used... should I systematically change them over to if key in dict?

[takluyver]

Can I suggest you just do the new feature on its own, and if you want to do substantial clean-ups, do them as a separate PR? It's fine to do a couple of small cleanups in the same function, but beyond that it's hard to see what's really changed.

Also, please test if key in dict rather than if key in dict.keys(). The former is perfectly standard Python, and also faster (looking up a key in a dict is O(1), whereas dict.keys() produces a list, and searching that is O(N)).

[andrewgiessel]

these are both excellent suggestions, thanks!

best,

ag

On Wed, Dec 28, 2011 at 13:24, Thomas <
reply@reply.github.com

wrote:

Can I suggest you just do the new feature on its own, and if you want to
do substantial clean-ups, do them as a separate PR? It's fine to do a
couple of small cleanups in the same function, but beyond that it's hard to
see what's really changed.

Also, please test if key in dict rather than if key in dict.keys().
The former is perfectly standard Python, and also faster (looking up a key
in a dict is O(1), whereas dict.keys() produces a list, and searching that
is O(N)).

---

Reply to this email directly or view it on GitHub:
#1210 (comment)

Andrew Giessel, PhD

Department of Neurobiology, Harvard Medical School
220 Longwood Ave Boston, MA 02115
ph: 617.432.7971 email: andrew_giessel@hms.harvard.edu

[andrewgiessel]

with this in mind, should we cancel this pull request? I can submit a new
one off a clean clone, unless you think i should just go and change these
to all to if key in dict and just go from there...

Related, if there is a best practice doc someplace, I'd be happy to read
it. I'm not looking to get a lesson in how to use git from you guys but I
appreciate the advice/help!

On Thu, Dec 29, 2011 at 20:14, andrew giessel andrew.giessel@gmail.comwrote:

these are both excellent suggestions, thanks!

best,

ag

On Wed, Dec 28, 2011 at 13:24, Thomas <
reply@reply.github.com

wrote:

Can I suggest you just do the new feature on its own, and if you want to
do substantial clean-ups, do them as a separate PR? It's fine to do a
couple of small cleanups in the same function, but beyond that it's hard to
see what's really changed.

Also, please test if key in dict rather than if key in dict.keys().
The former is perfectly standard Python, and also faster (looking up a key
in a dict is O(1), whereas dict.keys() produces a list, and searching that
is O(N)).

---

Reply to this email directly or view it on GitHub:
#1210 (comment)

Andrew Giessel, PhD

Department of Neurobiology, Harvard Medical School
220 Longwood Ave Boston, MA 02115
ph: 617.432.7971 email: andrew_giessel@hms.harvard.edu

Andrew Giessel, PhD

Department of Neurobiology, Harvard Medical School
220 Longwood Ave Boston, MA 02115
ph: 617.432.7971 email: andrew_giessel@hms.harvard.edu

[Carreau]

You don't need to cancel a Pull Request, you can force push a new set of commits by git push -f. Github will still track the new HEAD

[takluyver]

You can always replace the commits in a PR, but in this case I think it's cleaner to cancel it and start the discussion afresh on a new PR. Thanks, Andrew.

[takluyver]

Oh, and our docs on the development workflow: http://ipython.org/ipython-doc/stable/development/gitwash/index.html

[fperez]

@andrewgiessel, given that this PR doesn't merge anymore because of conflicts with changes in master in the meantime, and given the discussion above, it seems to me that the best solution is to close this one and for you to open a new one with the feedback you received above: use x in d but only clean up code right where you are actually working. Large, systematic cleanups should be done in their own PRs that don't otherwise change any functionality, so they are easier to review (and if you want to do that too, don't hesitate).

We appreciate your contribution, and once you get a hang of the process you'll see it goes pretty smoothly. Let us know if anything isn't clear and we'll be happy to help you.