Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upPython sqlite sqlalchemy #99
Conversation
|
I think the Chinook-app looks quite nice. The only big issue I found was a small logic error in detecting duplicates when adding new albums or tracks. I'm also wondering if there is a way to remind the readers that they need to add in the One way to do it could be to add a Take care! |
python-sqlite-sqlalchemy/project/examples/example_3/app/templates/base.html
Outdated
Show resolved
Hide resolved
python-sqlite-sqlalchemy/project/examples/example_3/app/tracks/routes.py
Outdated
Show resolved
Hide resolved
|
@writeson Sorry for the delay. I think this is a great update. Unfortunately, I found a few other bugs we need to straighten out. Also, I'll repeat the request to have something that reminds users to create a Take care! |
| return data.groupby("publisher") | ||
| .size() | ||
| .sort_values(ascending=ascending) |
This comment has been minimized.
This comment has been minimized.
gahjelle
Aug 13, 2020
Contributor
This is a syntax error. In the article, we use parentheses to allow for linebreaks.
However, Black will format this on one line since that line is still short enough to fit. I think it's okay to leave it on one line here in the repo.
Spending multiple lines in the article is probably good for readability though. I think it's also okay to keep that small difference as long as we feel it improves the readability of the code in the article.
| return data.groupby("publisher") | |
| .size() | |
| .sort_values(ascending=ascending) | |
| return data.groupby("publisher").size().sort_values(ascending=ascending) |
| data = get_author_book_publisher_data(csv_filepath) | ||
| author_book_publisher_data = data |
This comment has been minimized.
This comment has been minimized.
gahjelle
Aug 13, 2020
Contributor
I think this looks very smelly. I guess the reason you read into a temporary data variable and rename it later is to avoid Black adding a linebreak? I'd argue that having the linebreak is better - although I agree it's less than ideal - because then the code is straight-forward, it's just the formatting that's a bit off.
Seeing the current code, I stopped and started looking for how data would be used and trying to figure out why we need two names pointing to the same object.
In the examples, we have shortened these names, which could be done also here.
| db_path = base_path / "data" / "chinook.db" | ||
|
|
||
| SECRET_KEY = os.getenv("SECRET_KEY") | ||
| SQLALCHEMY_DATABASE_URI = f"sqlite:///{str(db_path)}" |
This comment has been minimized.
This comment has been minimized.
gahjelle
Aug 13, 2020
Contributor
str() is unnecessary here, the formatting converts the path to a string.
| SQLALCHEMY_DATABASE_URI = f"sqlite:///{str(db_path)}" | |
| SQLALCHEMY_DATABASE_URI = f"sqlite:///{db_path}" |
| [metadata] | ||
| name = local_project | ||
| version = 0.1.0 | ||
|
|
||
| [options] | ||
| packages = structure |
This comment has been minimized.
This comment has been minimized.
gahjelle
Aug 13, 2020
Contributor
I think I recognize this
However, we should either use setup.cfg and only a "shim" setup.py that calls setup() without any options, or only use a setup.py specifying all the options. The latter is probably the easiest. However, if you want to go with setup.cfg (which is somewhat recommended: https://snarky.ca/what-the-heck-is-pyproject-toml/) there are a few changes we should make:
packagesshould point to the directory of the code, so it would beprojectand possibly some of the subdirectories- You should also add a
install_requireskey in the[options]section listing the dependencies.
| book = session.query(Book).filter(Book.title == book_title).one_or_none() | ||
|
|
||
| # Get the publisher if exists | ||
| publisher = ( | ||
| session.query(Publisher) | ||
| .filter(Publisher.name == publisher_name) | ||
| .one_or_none() | ||
| ) | ||
| # Does book, author and publisher already exist? | ||
| if book is not None and author is not None and publisher is not None: | ||
| raise ValueError( | ||
| "New item exists", author_name, book_title, publisher_name | ||
| ) |
This comment has been minimized.
This comment has been minimized.
gahjelle
Aug 13, 2020
Contributor
Unfortunately, this seems to have the same duplication issue we found in the Chinook example. Different authors are not allowed to write books with the same name, or different publishers can't publish the same book.
>>> add_new_book(session, author_name="Tom Clancy", book_title="The Good Earth", publisher_name="Berkley")
ValueError: ('New item exists', 'Tom Clancy', 'The Good Earth', 'Berkley')
| # Get the artist | ||
| artist = ( | ||
| db.session.query(Artist) | ||
| .filter(Artist.artist_id == artist_id) | ||
| .one_or_none() | ||
| ) | ||
|
|
||
| form.artist.data = artist.name |
This comment has been minimized.
This comment has been minimized.
gahjelle
Aug 13, 2020
Contributor
If you click on Albums in the top menu, no particular artist is chosen, so artist will be None after the query and artist.name will raise an AttributeError: 'NoneType' object has no attribute 'name'.
I would probably have expected clicking on Albums to show me a list of all albums in the database. If that is not trivial to fix/implement, one option could be to simply remove Albums from the top menu. Accessing albums through the artist pages seem to work fine.
|
@writeson I made you a Didactic Review video. Please also check out this course. |
writeson commentedJan 20, 2020
Where to put new files:
my-awesome-articleHow to merge your changes: