JimLakis / PDM_Wildlife

0 stars 0 forks source link

code feedback 21st of July 2023 #2

Open bbelderbos opened 1 year ago

bbelderbos commented 1 year ago

Hey @JimLakis I looked at the code, great job writing code in the context of your app now.

Few things:

  1. I see you have code in a package called models and code in the main repo folder. I would consolidate this all in the package folder and maybe rename this folder to wildlife (because it will contain more than just models)
  2. I see headers with author + date in all files, this can be removed. This information is in Git version control.
  3. I have not seen a model being callable, what does this statement do models.Animals()?
  4. The block of code in create_database_initial_table.py you could put this in a function so it's importable.
  5. SQLModel.metadata.create_all(engine) I would put this in a separate function actually called create_tables
  6. You've hardcoded the database (sqlite_file_name = "animals_db.db"), it would be nice to load this from an environment variable see https://pybit.es/articles/how-to-handle-environment-variables-in-python/ (this can be done later)
  7. I wonder if the type hint is optional when you give it a default int? number_observed: Optional[int] = 1
  8. Nit pick but good to know: try to run your code by flake8 / black, e.g. table = True should be without spaces and as per PEP8 imports should be grouped: Standard Library -> external modules -> own modules

So:

from typing import Optional

from sqlmodel import Field, SQLModel

from datetime import datetime

Would become:

from datetime import datetime
from typing import Optional

from sqlmodel import Field, SQLModel

Great start. Please from here on, work with branches and open pull requests as per this PDM onboarding training: https://pybites.mykajabi.com/products/pybites-developer-mindset-pdm-program/categories/2560320/posts/9297325

Thanks, Bob

bbelderbos commented 1 year ago

@JimLakis any questions or doubts on this feedback?