Description
I ran just lesson 1 for a group who turned out to be slightly above novice but turned out to be not quite at the right level for this, so these comments may not be quite right.
The episode on code style conventions exercise on improving the code style of our project suffers from the code already being fairly well-written. There's only a tiny number of errors to fix, and other than changing the InFiles
variable name they don't really 'sell' the value of code conventions - the code doesn't become appreciably clearer or better as a result.
I'm biased, but I think perhaps the Code Style section could open up with something like the Naming Things section of the 'Managing an Academic Software Project' material from Soton, covering good naming, then roll from there into "There are formal standards for how to write clear code in Python!", and start that block off with Naming Conventions and Whitespace, rather than line breaks and indentation.
The code that we fix the style of should also be worse. More errors, and clearer errors, that more immediately impact consistency and readability. As it stands, PEP8 ends up feeling like an exercise in nitpicking.
This is also the case for the PEP257 bit, where the docstrings are already somewhat descriptive, and end up much longer than the actual function. The value of the Sphinx-style docstrings is undersold when there isn't a link or example of how they can be used to autogenerate documentation. The docstrings should be missing entirely for one or more functions, as it's easy to figure out what they should be.
The episode on linting has similar issues - it raises very few problems, of which most are invisible and so fixing them will have no practical effect. It makes linting seem a bit arbitrary. This episode would benefit from models.py
getting scuffed up a bit too. It's a bit confusing as well as PyCharm is already linting.
On a less opinionated note - the Integrated Software Development Environments episode runs into some issues as PyCharm automatically recognises the venv
directory as a project interpreter, and automatically generates a run configuration. Editing the run configuration to add an actual data file would be a good change, even if we don't want to run it in this episode. It also introduces how to install dependencies via PyCharm, but then has you manually update requirements.txt
- it might be nice to introduce PyCharm's requirements management.
Proposed alternate structure (from brief chat with Steve):
- Code Standards
- Start off with a bit on good variable names
- Explain how clear, standard names and formats make code easier to read
- Refactor poor/unclear names in
inflammation_analysis.py
- Use PyCharm refactoring
- Introduce PEP8 basics
- We don't need to go into much detail as the linter handles it
- Linting
- Using PyCharm default linter to spot some basic problems and correct them
- Using PyLint on the command line
- This highlights lack of docstrings
- Introduce PEP257 basics, show examples of Sphinx docs
- Show parsing of docstrings by PyCharm
- Exercise to lint and improve
models.py
- Optional: Add code formatters? Auto-formatting via Pycharm?
Changes suggested for the base material:
- General worsening of code quality in
inflammation_analysis.py
, e.g.- Inconsistent/unclear variable names (e.g. replace
view_data
withvd
) - Poor function naming (e.g. replace
main
withMain
)
- Inconsistent/unclear variable names (e.g. replace
- General worsening of code quality in
models.py
, e.g.- Remove comments from
models.py
- Poor function naming (e.g. replace
load_csv
withload
) - Introduce potential name collisions (e.g. replace
daily_max
withmax
)
- Remove comments from
As these changes are fixed in the exercises, they don't require any changes in the later episodes.