You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 with vd)
Poor function naming (e.g. replace main with Main)
General worsening of code quality in models.py, e.g.
Remove comments from models.py
Poor function naming (e.g. replace load_csv with load)
Introduce potential name collisions (e.g. replace daily_max with max)
As these changes are fixed in the exercises, they don't require any changes in the later episodes.
The text was updated successfully, but these errors were encountered:
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 updaterequirements.txt
- it might be nice to introduce PyCharm's requirements management.Proposed alternate structure (from brief chat with Steve):
inflammation_analysis.py
models.py
Changes suggested for the base material:
inflammation_analysis.py
, e.g.view_data
withvd
)main
withMain
)models.py
, e.g.models.py
load_csv
withload
)daily_max
withmax
)As these changes are fixed in the exercises, they don't require any changes in the later episodes.
The text was updated successfully, but these errors were encountered: