-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
AmyOctoCat
commented
Jun 20, 2024
•
edited
Loading
edited
- 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
…matology.py and making some suggestions for other possible naming changes. Removed use of assert and slight refactor to the convert_pr_units function to add an explaining variable.
…ce is here. t Please enter the commit message for your changes. Lines starting
There was a problem hiding this 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.
Co-authored-by: jatkinson1000 <[email protected]>
…-ICCS/rse-skills-python into add_section_on_variable_naming
Co-authored-by: jatkinson1000 <[email protected]>
…lengthy for this iteration
…xercises (blank lines not picked up by black).
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
Patch whitespace on naming exercise updates
There was a problem hiding this 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.