Add database to project #47
Conversation
…lbum/playlist data.
|
@JackMead Realise some refactoring might be due here. Thought I'd save that for a following PR to reduce the number of changes you have to muddle through. |
|
(Note to self to also review format-on-save) |
There was a problem hiding this comment.
I've not done a thorough review but it looks great! Well done finding the time to get all of this together, and it's really nice seeing it start to build out some real advantageous new functionality 😄
Some more detailed thoughts on comments below, and:
- As we chatted about, I think some pulling it into controller/service/maybe even repository level structure would help separate out the different concerns
- As mentioned below, some guidance for how to run migrations would be good
- And also I fell into a trap trying to run it with Docker without the latest CSS being generated, so potentially worth making sure the dev container at least compiles the latest css (or even runs that in hot-reload mode too?)
| @@ -1,15 +1,19 @@ | |||
| from flask import Flask, make_response | |||
There was a problem hiding this comment.
On autoformatting:
- I can't remember the exact details we discussed, but what I see is that auto-formatting works in JS but not in Python, which I think matches what you see
- First I checked that I do have a Python formatter installed, I actively added the Black-formatter extension from Microsoft but there was a default one already
- But looking at the Black formatter output (in the "output" tab of the terminal)
2024-09-13 11:09:15.445 [info] The language client requires VS Code version ^1.82.0 but received version 1.79.2
Updating my VSCode (and it was about time I did!) fixed the issue, so maybe you're seeing the same?
There was a problem hiding this comment.
In fact it's actually the JS side that's not working for me 😁 . I'll have a bit more of a look into it. Aware that trying to get it in in this PR might cause a lot of unnecessary formatting changes to clutter up the actual changes.
| from time import sleep | ||
|
|
||
|
|
||
| def database_controller(spotify: SpotifyClient, musicbrainz: MusicbrainzClient): |
There was a problem hiding this comment.
Very minor, but conceptually this name bugs me because it's very much exposing internal details as API details... but I can't immediately offer better! The conceptual tie between the things we own vs Spotify is strong
I'll leave this as a thought, but I wouldn't worry about changing it
There was a problem hiding this comment.
Yeah, it doesn't quite seem right. I think once I've got the database management happening independently I can get rid of it entirely. Will leave as is for now though.
| @@ -0,0 +1,98 @@ | |||
| from typing import List | |||
There was a problem hiding this comment.
(I'm definitely not properly reviewing all of these models just fyi 😄 )
Adds database setup and connections to project. Includes new musicbrainz connection for getting information on album genres if available (since Spotify appears to have none).
Adds a basic settings page to the frontend for populating user data.