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

Lesson 1 Content #388

Open
smangham opened this issue Oct 9, 2024 · 0 comments
Open

Lesson 1 Content #388

smangham opened this issue Oct 9, 2024 · 0 comments

Comments

@smangham
Copy link
Collaborator

smangham commented Oct 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant