Adding context manager into thw-python strings-io introduction

[leszektarkowski]

Using context managers is a very good general practice, allowing to get rid of many lurking bugs.

I hope this basic introduction will encourage fellow coders to use it more often.

[rgaiacs]

Since "f" is the name of a variable in the example will be nice using f.

What about replace "creating file handle called f" with "creating file handle called, in the example, f"?

[mattphotonman]

creating file handle -> creating a file handle

[leszektarkowski]

Thanks - I'm updating commit according to your comments.

[rgaiacs]

@czterybity Very nice job. Context manager is very useful and are missing in this tutorial.

Just take a look at my comment in line 302. After that this PR have my approve.

[wking]

On Mon, Oct 21, 2013 at 02:03:42PM -0700, Leszek Tarkowski wrote:

Using context managers is a very good general practice, allowing to
get rid of many lurking bugs.

Hooray context managers :).

• using context manager in files tutorial
• using context manager in files tutorial
• typo fixed
• Merge remote-tracking branch 'upstream/gh-pages' into gh-pages

I like the content, although there are some grammar mistakes (for
example “In less trivial scenarios file should be closed after using
to prevent data corruption.” should read “In less trivial scenarios
the file should be closed after use to prevent data corruption.”).
I'd also like to see a link to PEP 343
(http://www.python.org/dev/peps/pep-0343/) for anyone who wants more
details.

On the PR-mechanics front, there is no need to merge your pull request
before submitting it. After the pull request is accepted, your branch
will be merged into master. You've also got some odd internal
merging:

*   7cd2980 (gvwilson-new/pr/90) Merge remote-tracking branch 'upstream/gh-pages' into gh-pages
|\  
| * a64b44d (gvwilson-new/gh-pages) a couple more corrections to top-level README
| …
| |
*    \  4157357 typo fixed
|\  |
| * | 7dd72ad using context manager in files tutorial
* |/ 3756d05 using context manager in files tutorial
|/  
*   985af7f Merge pull request #59 from r-gaia-cs/teach06-expansion

Where it looks like 7dd72ad and 3756d05 are almost identical, except
that 3756d05 has “trivial” where 7dd72ad has “tivial”. You can
cleanup your Git history and update this pull request by running:

$ git checkout gh-pages
$ git reset --hard 3756d05
$ git push -f czterybity gh-pages

replacing czterybity with whatever you've called your public
repository. This will drop 7dd72ad, 4157357, and 7cd2980 from your
branch, since they contain no useful changes and only confuse the
thrust of your changes.

In the future, it's also good practice to develop changes in
appropriately named feature branches, because it's not immediately
apparent what changes gh-pages holds. For example:

$ git checkout -b context-managers origin/gh-pages

will create a new context-managers branch and check it out. Then
we'd have a better idea of what that branch held without looking at
the individual commits.

If you need help with any of the Git manipulations, or want more help
proofreading, let me know…

[ahmadia]

@wking and @r-gaia-cs - thanks for the detailed reviews and feedback.

@wking - Can I put you in charge of helping this PR get integrated into gh-pages?

[wking]

On Mon, Oct 21, 2013 at 06:55:14PM -0700, Aron Ahmadia wrote:

@wking - Can I put you in charge of helping this PR get integrated
into gh-pages?

I think per-section maintainers would probably be a better approach
than per-pull-request shepherds. I haven't actually looked at
anything else in this section yet, so I'm not sure how this addition
fits into the bigger picture. For example, are we trying to stay
synchronized with tutorial.ipynb? Is this section running long?
Short? Glazing eyes with exhaustion? Boredom? I can evaluate the PR
as an individual change, but I'm guessing fairly blindly on the other
fronts. In the absence of a maintainer, I'll do my best, but maybe
@guyrt would be a better choice as the original author of this
section
(swcarpentry/DEPRECATED-boot-camps@12d3bf7) or
@scopatz as the author of the notebook form (534a906). Other folks
who have taken an interest in this lesson include @gvwilson (6e7b321),
@pipitone
(swcarpentry/DEPRECATED-boot-camps@4ba1cf2), and
@katyhuff
(swcarpentry/DEPRECATED-boot-camps@8c8ace8).
I'm happy to coach @czterybity through the proofing and Git hygiene,
but they may be better positioned for the bigger picture decisions.

[mattphotonman]

due to a fact -> due to the fact

[leszektarkowski]

Thanks.

[mattphotonman]

I made a bunch of minor grammar corrections, I hope it's not too messy!

[gvwilson]

The current goal is to get instructor trainees comfortable with writing
and GitHubbing; as soon as this exercise is over, we'll do a pass to
smooth things out, so for now, it's OK to focus on internal consistency
(grammar, example code does what the text says it does, etc.).

[ahmadia]

@wking - at this point, we need coaching more than consistency. Thanks for flagging the originators of this material, I'll rely on your best judgement for all of the issues you mentioned.

[wking]

On Tue, Oct 22, 2013 at 01:58:21AM -0700, Greg Wilson wrote:

The current goal is to get instructor trainees comfortable with
writing and GitHubbing; as soon as this exercise is over, we'll do a
pass to smooth things out, so for now, it's OK to focus on internal
consistency (grammar, example code does what the text says it does,
etc.).

A hit list of documentation shortcomings might be useful to guide
future modification efforts. I just have the feeling that
with-statements wouldn't make the cut for a two-day workshop, unless
you used them as an example of “don't reinvent the wheel” or “learn to
write idomatic $LANGUAGE, it helps avoid errors and makes it easier
for others to understand your code”. In that case, perhaps we want to
rework the text to emphasize those big-picture goals? We can
certainly revisit this question once we get an internally consistent
PR here (thanks for the detailed suggestions, @mattphotonman), but it
would be a shame to spend time polishing in a dead-end direction.

[wltrimbl]

I'm with @wking, as nice as this syntax is, I don't think it fits in the two-day bootcamp format.
Would drawing attention to the scoping of variables in functions would be more fruitful use of our learners' attention?

[ethanwhite]

I agree with @wking and @wltrimbl that this is too advanced for bootcamps. As more advanced material for online learners I think it would be a great addition.

[wking]

On Mon, Oct 21, 2013 at 06:21:14PM -0700, W. Trevor King wrote:

$ git checkout gh-pages
$ git reset --hard 3756d05
$ git push -f czterybity gh-pages

@czterybity, it looks like you got this done. Congrats :). For typo
fixes and such I'd recommend using commit --amend … to squash them
onto the original commit, because history like

• Fix another typo
• Fix a typo
• Initial implementation

is not generally helpful. Commits that encapulate a single semantic
change and explain why it was called for are more useful, and avoiding
typo-fixing commits increases the signal-to-noise in the Git history.

@mattphotonman has pointed out a number of possible improvements, but
you might want to wait on that until the bigger picture issue of where
to put these notes has been settled before wading in on that front. I
agree with @ethanwhite that this would be useful for long-form
web-only notes, which is the role I envisioned for the instructor
guide 1. I don't think we reached a consensus in the broader SWC
community though.

[gvwilson]

On 2013-10-22 1:29 PM, W. Trevor King wrote:

A hit list of documentation shortcomings might be useful to guide
future modification efforts.
Agreed --- I'll be asking instructors of recent bootcamps to put that
together, since they're the ones with the most realistic view of how far
we're getting and what we should be covering.

[leszektarkowski]

Many thanks for @wking for git usage help and @mattphotonman for language corrections.
But ( @wltrimbl and @ethanwhite ) I do not agree that using context managers is something too advanced for a bootcamp. Upon reaching consensus here I can rewrite whole example to use it from the beginning. I think that using CM is Python best-practice. And it does not complicate things (except - maybe - trying to understand how they work internally)

[ahmadia]

@czterybity - Context managers are an awesome concept, but remember that teaching students the why of them rely on the students (all of them, not just the ones who've taken programming classes) having a concept of:

• file handles as objects
• resource acquisition is initialization

before we start dealing with the semantics of the with statement. I think we all agree that this material is useful and we want it in bc. I think the current decision we're trying to make is where to put it :)

[leszektarkowski]

@ahmadia - my point is that maybe we should use them consistently, as "that's the way you should do it", without detailed explanation. Semantics is trivial, it may be actually easier for someone to grasp this with this_object as name_of_object: do some stuff. My experience shows that = can be equally confusing if taken literally (for math people a = a+1 is a blasphemy :)

[ahmadia]

@czterybity - Okay, point taken. I think that I (or somebody else) needs to take a more detailed look at how we're using open for files through the lessons and paint a broader picture here than the diff currently does.

This is @wking's show, so I'll try to take a step back here and let him manage this. You're in good hands :)

[gvwilson]

I've always used 'open' because (a) that's still what most Python docs
do, (b) open/close is what learners will see in other languages, and (c)
when I introduce 'with', I get students asking why they can't write:

 with numpy.loadtxt('mydata.csv', delimiter=',') as data:
     print data.mean()

[leszektarkowski]

@gvwilson - you got the point about other languages. But - IMHO your example is a numpy fault. In Python3 (our bright future) even successor of urllib2 - urllib.requests is using CM. threading.Lock is using it since 2.6.

But you are probably right. My experience is flawed by a fact that "my" Python courses are lasting at leas three days (usually five).

[ethanwhite]

I agree with @gvwilson's points and would also add that in my experience
asking students to "just trust me, this is how it works" needs to be kept to
an absolute minimum. This is a case where 'open' makes easy intuitive
sense, and 'with' doesn't. Since 'open' also makes more sense outside of
Python I think it's the better choice for short courses.

Again, I think this is the right thing to use and something we should provide
online, I'm just not convinced that it works in a 2 day bootcamp.

[wking]

On Tue, Oct 22, 2013 at 03:11:03PM -0700, W. Trevor King wrote:

@mattphotonman has pointed out a number of possible improvements,…

For those following along at home, the difference between the original
PR's 3756d05 and the current b857d76 is:

$ git diff --word-diff 3756d05..bc/pr/90
diff --git a/lessons/thw-python/strings-io/tutorial.md b/lessons/thw-python/strings-io/tutorial.md
index 2985599..13628c4 100644
--- a/lessons/thw-python/strings-io/tutorial.md
+++ b/lessons/thw-python/strings-io/tutorial.md
@@ -284,23 +284,23 @@ each string in the list, add it yourself.
Context manager
---------------

Closing a file is something often neglected in Python, due to [-a-]{+the+} fact that
it is done automatically at the end of a script (garbage collection).

In less trivial scenarios {+the+} file should be closed after [-using-]{+use+} to prevent data
corruption. To be sure of that you can use {+a+} special language construct
called {+a+} context manager (available since almost ancient Python 2.5).

```python
   with open('outfile.txt','w') as f:
       f.write("Message of a Great Importance")

   [-pass #another-]{+#other+} instructions, file is already closed at that point
```

Also called "with statement", context manager [-will be-]{+is+} responsible for
opening a file, creating {+a+} file handle [-called f,-]{+called, in the example `f`,+} and after
a block of [-instructions it will close-]{+instructions, for closing+} the file.

Still to go are those mentioned in:

• Adding context manager into thw-python strings-io introduction #90 (comment)
• Adding context manager into thw-python strings-io introduction #90 (comment)

[ahmadia]

@wking @czterybity - sorry to interject, but I think the consensus so far is that we should make this lesson available (perhaps even expand it), but not locate it within the base material. I agree that we should get +1 from @scopatz before moving it into the notebook. If he feels that it belongs somewhere else (or you don't want to wait for him), we can create a new top-level [tag]-python/context-managers, where tag could either be [thw], [swc], or [czterybity], depending on whether you'd like to seat it one of the lessons or make it standalone.

Either way, I'm going to deprecate the non-notebook form of the Markdown file in a separate issue, since we discussed doing this awhile ago and never did.

[leszektarkowski]

@ahmadia - thanks for clarification.

[wking]

With the current c5a75c5, changes are:

$ git diff --word-diff b857d76..c5a75c5
diff --git a/lessons/thw-python/strings-io/tutorial.md b/lessons/thw-python/strings-io/tutorial.md
index 13628c4..d5ed7ed 100644
--- a/lessons/thw-python/strings-io/tutorial.md
+++ b/lessons/thw-python/strings-io/tutorial.md
@@ -287,8 +287,8 @@ Context manager
Closing a file is something often neglected in Python, due to the fact that
it is done automatically at the end of a script (garbage collection).

In less trivial scenarios [-the-]{+a+} file should be closed after [-use-]{+using it+} to prevent
data corruption. To be sure of that you can use a special language construct
called a context manager (available since almost ancient Python 2.5).

```python

That's the change suggested here 1, but after the the change I
suggested here 2 went in between 3756d05 and b857d76, I actually
like the b857d76 form better.

On Wed, Oct 23, 2013 at 01:53:00PM -0700, W. Trevor King wrote:

Still to go are those mentioned in:

• Adding context manager into thw-python strings-io introduction #90 (comment)
• Adding context manager into thw-python strings-io introduction #90 (comment)

These are still missing from c5a75c5. For clarity:

• To be sure of that, you can use a special language construct [new
  comma]
• Also called "with statement" -> Also called "with statements" [new
  plural]
• context manager will be responsible -> context managers are
  responsible [new plural and verb conjugation]

[ahmadia]

@czterybity @wking - Would you like this flagged for review? Do you need any other help here?

[wking]

On Sun, Nov 03, 2013 at 05:37:59PM -0800, Aron Ahmadia wrote:

Would you like this flagged for review? Do you need any other help
here?

I think we're ok. Outstanding issues with the current c5a75c5 are
outlined here, and I'm just waiting for @czterybity to address
them. I'm still not clear about where this will live in the new
layout (#118, #128), but I'm happy to merge it (once we fix the
outstanding issues) if we're focusing on internal consistency and
clarity.

[gvwilson]

I propose merging this in, then letting @ethanwhite decide if it belongs in the python/intermediate tutorial.

[ahmadia]

@gvwilson - this one still is waiting on fixes from @czterybity.

[gvwilson]

@czterybity, will you have a chance to do fixes in the next couple of days? We'd like to tag the repo... (I'm OK with this stuff landing where it is, and being copied/moved to the python/intermediate lesson when it materializes.)

[leszektarkowski]

I'll do it this weekend. Sorry about the delay.

[ahmadia]

Thanks Leszek, please have it in by Saturday, if possible.

On Thu, Nov 28, 2013 at 3:05 PM, Leszek Tarkowski
notifications@github.comwrote:

I'll do it this weekend. Sorry about the delay.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/90#issuecomment-29483143
.

[ahmadia]

@czterybity - as far as I can tell, this is still waiting on fixes from you. Please ping back if you have any questions or if I can help in some way.

[leszektarkowski]

Hmm, I thought I had it all fixed yesterday. I'll take a look in the morning.

[ahmadia]

@czterybity - It looks like you're suggesting leszektarkowski@e9ca532 as the PR.

It's missing (from a cursory scan), the suggested fixes by wking here: #90 (comment)

I raised an earlier objection about this landing in our tutorial materials, but since the THW material serves a slightly more advanced audience, I'm okay with landing this in for the tag.

[leszektarkowski]

I closed my pull request by mistake... It's always bad to do something with console being tired.

[ahmadia]

Reopened :)

[leszektarkowski]

Magic. I think right now it contains all the requested changes.

[ahmadia]

Okay, I'll take a look at bringing this in tonight.

[wking]

On Sun, Dec 01, 2013 at 02:58:05PM -0800, Aron Ahmadia wrote:

Okay, I'll take a look at bringing this in tonight.

This looks good to me now, but the file it's landing in is gone since
#165. Should we integrate the changes in the IPyNb file?

[ahmadia]

Yes, that's what needs to happen :) I was planning on adding a merge commit bringing the changes in and deleting this file.

[wking]

On Sun, Dec 01, 2013 at 04:45:56PM -0800, Aron Ahmadia wrote:

Yes, that's what needs to happen :) I was planning on adding a merge
commit bringing the changes in and deleting this file.

Sounds good to me.

[ahmadia]

Merged in manually in e74d529

[leszektarkowski]

Many thanks to all of you. See you next time :)