Implenet new datamodel on the backend #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andyjpb
suggested changes
Aug 15, 2024
There was a problem hiding this comment.
A nice start! :-)
Just a few actions:
- Separate out stream refactoring into its own commit.
- Separate out structural model stuff into its own commit.
- Separate out user data model stuff into its own commit.
- Separate out routes and stuff into their own commit(s) that build on the groundwork from the structural model commit.
- Provide a commit full of tests / example data to fill up the structural model: at least a few cubes with a few different dimensions, revisions and sources that demonstrate all the functionality so far.
- Provide commit messages for all commits to explain rationale, design, progress so far, limitations, assumptions and general documentation for what we're thinking.
This now makes better use of streams. It completes the use of the new data model with end points for viewing the dataset and getting a preview of the uploaded datafile.
j-maynard
changed the title
There was a problem hiding this comment.
Looks pretty good, I've added quite a few comments but some of it is for my understanding and the rest are bitty things that should be easy to remedy.
If you find I'm too pedantic let me know and I can tweak the levels down a bit 😄
Some general comments:
- We're missing return types on most functions - is this something we want to start getting into the habit of providing? If so, we can add a lint warning when they're missing?
- This is just semantics but I'd say
BlobStorageServiceandCSVProcessorare both services rather than controllers. Do we want to makeCSVProcessora class also? - The switching of column types based on
environment == testfeels a bit strange, I'm assuming it's to do with some issue you had using the proper types when testing? I've not had to do this before so maybe we can resolve it with test config or mocks? - There are currently no indexes on any of the entities, we might want to do a pass looking at what fields are used in queries and adding indexes on those fields.
j-maynard
force-pushed
the
SW-95-implement-new-data-model
branch
from
bbda21e to
29db52e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make all the changes to the backend to support the new database model.