Skip to content

[pthon_essentials] Adding context managers (with statements)#253

Merged
jstac merged 6 commits intomainfrom
add-with
Dec 2, 2022
Merged

[pthon_essentials] Adding context managers (with statements)#253
jstac merged 6 commits intomainfrom
add-with

Conversation

@HumphreyYang
Copy link
Copy Markdown
Member

Hi @mmcky,

This PR addresses #247. Could you have a quick look at the additions when the preview is ready and merge if it looks good to you?

Many thanks in advance.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 21, 2022

@HumphreyYang HumphreyYang requested a review from mmcky November 21, 2022 10:09
Copy link
Copy Markdown
Contributor

@mmcky mmcky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HumphreyYang just made a few suggestions.

My only other comment is that I can't remember a time whereI have ever written and read from a file in the same with statement before. I just wonder if this is overly complicating the example. I typically use a+ to append to an already established file. Thoughts?

Comment thread lectures/python_essentials.md Outdated
print(out)
```

We can also use `with` statement to contain operations on the file within a block for clarity and cleanness.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
We can also use `with` statement to contain operations on the file within a block for clarity and cleanness.
We can also use a `with` statement (also known as a [context manager](https://realpython.com/python-with-statement/#the-with-statement-approach)) which enables you to group operations on the file within a block which improves the clarity of your code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is too long and uses "which" twice.

Please remember that there must always be a comma before which. ("...within a block, which...")

"We can also use..." is the start of the previous sentence in the file so it's better not to reuse it.

Comment thread lectures/python_essentials.md Outdated
print(f'Line {i}: {line}')
```

Note that we used `a+` mode (standing for append+ mode) to allow appending new content at the end of the file and enable reading at the same time.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Note that we used `a+` mode (standing for append+ mode) to allow appending new content at the end of the file and enable reading at the same time.
```{note}
We used `a+` mode (standing for append+ mode) to allow appending new content at the end of the file and enable reading at the same time.
```

@HumphreyYang HumphreyYang marked this pull request as draft November 22, 2022 02:12
@HumphreyYang
Copy link
Copy Markdown
Member Author

@HumphreyYang just made a few suggestions.

My only other comment is that I can't remember a time when I have ever written and read from a file in the same with statement before. I just wonder if this is overly complicating the example. I typically use a+ to append to an already established file. Thoughts?

Hi @mmcky,

Thanks for your detailed review. I was overcomplicating the example to show that there are other modes that are capable of doing operations other than r and w modes. I think the standard practice is always to have a with statement to write and a with statement to read. Just curious if that is what you would suggest (i.e. breaking this down into two with statements).

@mmcky
Copy link
Copy Markdown
Contributor

mmcky commented Nov 22, 2022

@HumphreyYang just made a few suggestions.
My only other comment is that I can't remember a time when I have ever written and read from a file in the same with statement before. I just wonder if this is overly complicating the example. I typically use a+ to append to an already established file. Thoughts?

Hi @mmcky,

Thanks for your detailed review. I was overcomplicating the example to show that there are other modes that are capable of doing operations other than r and w modes. I think the standard practice is always to have a with statement to write and a with statement to read. Just curious if that is what you would suggest (i.e. breaking this down into two with statements).

thanks @HumphreyYang I would usually go down that path -- but I am a conservative programmer. I think though this would be good practice.

@mmcky mmcky changed the title Adding Intro to With Statement [pthon_essentials] Adding context managers (with statements) Nov 23, 2022
@jstac
Copy link
Copy Markdown
Contributor

jstac commented Nov 27, 2022

Please let me know when to review @mmcky and @HumphreyYang .

We can keep this addition to the lectures very simple and straightforward. Our only aim is to get people up to speed so that they can code well enough to handle subsequent econ lectures.

@HumphreyYang
Copy link
Copy Markdown
Member Author

Hi @mmcky,

I have pushed a new version with the section on the pointer removed entirely since it will inevitably lead to complicated examples with many seek and tell printings.

@HumphreyYang
Copy link
Copy Markdown
Member Author

Please let me know when to review @mmcky and @HumphreyYang .

We can keep this addition to the lectures very simple and straightforward. Our only aim is to get people up to speed so that they can code well enough to handle subsequent econ lectures.

Thanks for the feedback @jstac, I will adjust the style to fit into this objective.

@HumphreyYang HumphreyYang requested a review from mmcky November 27, 2022 23:09
Comment thread lectures/python_essentials.md Outdated
```{code-cell} python3
with open("newfile.txt", "r") as f:
file = f.readlines()
with open("output.txt", "w") as fo:
Copy link
Copy Markdown
Contributor

@mmcky mmcky Nov 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HumphreyYang should block be indented to be in the open file context?

Copy link
Copy Markdown
Contributor

@mmcky mmcky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @HumphreyYang it is simpler.

Just had one question re: indentation

@HumphreyYang
Copy link
Copy Markdown
Member Author

thanks @HumphreyYang it is simpler.

Just had one question re: indentation

Thanks for picking that out @mmcky. I have added an indentation to the block. Should we invite John for review if it looks good to you?

@mmcky mmcky marked this pull request as ready for review November 28, 2022 01:47
@mmcky mmcky requested a review from jstac November 28, 2022 01:47
@jstac
Copy link
Copy Markdown
Contributor

jstac commented Nov 30, 2022

Hi @mmcky , it says above "This branch has not been deployed. No deployments." I can't see anything related to netlify.

Am I missing something?

@mmcky
Copy link
Copy Markdown
Contributor

mmcky commented Nov 30, 2022

The last box in the checks section of the PR (need to scroll down as its the last item in the list) contains the Netlify link to the latest green tick deployment.

The Netlify integration used to post a link each run but not sure why that behaviour changed.

https://6383fedc52896355884dbec5--epic-agnesi-957267.netlify.app/intro.html

That not deployed message (as I understand) is if we used netlify to actually host he site it would confirm the deployment status. It is a bit misleading.

@jstac
Copy link
Copy Markdown
Contributor

jstac commented Nov 30, 2022

need to scroll down as its the last item in the list

Ah, that's where I was going wrong. To be fair, it's a bad idea to have the downward scroll indicator hidden until you hover...

Thanks @mmcky :-)

@jstac
Copy link
Copy Markdown
Contributor

jstac commented Nov 30, 2022

Hi guys, I thought the point of the with statement is: "The with statement itself ensures proper acquisition and release of resources." See https://www.geeksforgeeks.org/with-statement-in-python/

Perhaps this should be mentioned directly, so people can see the point.

Also, I recommend that we start with the simple example

f = open('newfile.txt', 'r')
out = f.read()
out

and show how that changes when with is used. It's usually best to start with the simplest example possible.

Then, after that, you could say: "Now suppose that we want to read input from one file and write output to another. Here's how we could do accomplish this task while correctly acquiring and returning resources to the operating system:"

fo.write(f'Line {i}: {line} \n')
```

```{code-cell} python3
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use with here, given that we just recommended it?

fo.write(f'Line {i}: {line} \n')
```

```{code-cell} python3
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use with here, as above?

@jstac
Copy link
Copy Markdown
Contributor

jstac commented Dec 1, 2022

Great work @HumphreyYang . This is very nice.

Just this comment to attend to:

"We can also use..." is the start of the previous sentence in the file so it's better not to reuse it.

How about "In fact, the recommended approach in modern Python is to use a with statement..."

@HumphreyYang
Copy link
Copy Markdown
Member Author

Member

Thanks for the feedback @jstac. It does sound much better. I have updated it accordingly.

Many thanks.

@jstac
Copy link
Copy Markdown
Contributor

jstac commented Dec 2, 2022

Thanks again @HumphreyYang , very nice examples. Merging.

@jstac jstac merged commit 3d6b065 into main Dec 2, 2022
@jstac jstac deleted the add-with branch December 2, 2022 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants