Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add section on variable naming #7

Merged
merged 45 commits into from
Jul 3, 2024
Merged

Conversation

AmyOctoCat
Copy link

@AmyOctoCat AmyOctoCat commented Jun 20, 2024

  • Add slide on variable and method naming
  • Add caveat around logging on slide with f-strings
  • Make some suggestions for potential edits to the naming in the exemplar python file
  • Add an exercise to illustrate naming
  • Update other exercises to incorporate naming changes

Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Hi @AmyOctoCat

This looks fantastic, thank you.
A couple of comments/suggestions for your consideration.

One thing that does come across is how these changes drastically improve the readability and understandability of the code. I wonder if it is worth putting this earlier in the workshop, perhaps even before linting (though I am not sure) to help people ease in and understand the code. Perhaps we can have a quick meeting together with @MarionBWeinzierl to think about this.

exercises/00_final/precipitation_climatology.py Outdated Show resolved Hide resolved
exercises/00_final/precipitation_climatology.py Outdated Show resolved Hide resolved
exercises/00_final/precipitation_climatology.py Outdated Show resolved Hide resolved
exercises/00_final/precipitation_climatology.py Outdated Show resolved Hide resolved
slides/_fstrings_magic_config.qmd Outdated Show resolved Hide resolved
slides/_fstrings_magic_config.qmd Outdated Show resolved Hide resolved
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Honestly, this is fantastic.

A couple of comments, and (amazingly!!) the only issue I can find when running through the exercises are 3 blank lines that need removing.
I have opened a PR #13 that would fix this.

I think there are some aesthetic changes to be made to the slides before they are ready to be used. Please do get in touch if you need help generating and viewing them.

exercises/03_naming/README.md Show resolved Hide resolved
slides/_formatting_with_black.qmd Outdated Show resolved Hide resolved
exercises/06_better_code/precipitation_climatology.py Outdated Show resolved Hide resolved
exercises/00_final/precipitation_climatology.py Outdated Show resolved Hide resolved
exercises/04_linting/precipitation_climatology.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Aside from one comment your additions here seem great!
vimdiff is a really nice way of comparing the changes, and works really well here - if you are not familiar and want to use in the exercises I can show you tomorrow.

slides/_naming_for_clarity.qmd Show resolved Hide resolved
slides/_naming_for_clarity.qmd Outdated Show resolved Hide resolved
slides/_naming_for_clarity.qmd Outdated Show resolved Hide resolved
slides/_naming_for_clarity.qmd Outdated Show resolved Hide resolved
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Thanks @AmyOctoCat This is a fine addition.

Added a small commit to make the bullets on naming incremental for you.

@jatkinson1000 jatkinson1000 merged commit bba918a into main Jul 3, 2024
1 check passed
@jatkinson1000 jatkinson1000 deleted the add_section_on_variable_naming branch July 3, 2024 10:24
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.

2 participants