Mtcars services fix #23
Merged
Conversation
This file contains hidden or 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
…ing into services, attempted to add new model for whole mtcars dataset but that still wont pass integration test
…coding from services as it was not needed with new method, removed mtcarlist data model
removed extra newline
jdhoffa
approved these changes
Feb 27, 2025
Collaborator
jdhoffa
left a comment
jdhoffa
left a comment
There was a problem hiding this comment.
Reviewed and tested locally, looks and works great!
Thanks @samhuestis
jdhoffa
reviewed
Feb 27, 2025
Comment on lines
+13
to
+19
| try: | ||
| # replaces the space in the model of each row, so it will be a valid url later | ||
| row["model"] = row["model"].replace(" ", "") | ||
| # validates row of csv against mtcar model | ||
| validated_row = mtcar.model_validate(row) | ||
| # dumps individual validated "mtcar" into list mtcars_data | ||
| mtcars_data.append(validated_row.model_dump()) |
Collaborator
There was a problem hiding this comment.
I'm happy to merge this as-is, but it would be interesting to see what the performance trade-offs are with validating each row on read.
Something to look into in the future
jdhoffa
reviewed
Feb 27, 2025
src/services/mtcars.py
Outdated
| mtcars[key] = row | ||
| return mtcars | ||
| # Function to read in csv, validate using mtcar model and output mtcars data in JSON | ||
| def csv_to_dict(mtcars_path): |
Collaborator
There was a problem hiding this comment.
Given that this function is no longer a generalized csv_to_dict function, but now very precisely an mtcars_vsc_to_dict() function (given it validates the input against the mtcars pydantic model) , I would suggest renaming the function to be more accurate.
jdhoffa
reviewed
Feb 27, 2025
jdhoffa
reviewed
Feb 27, 2025
jdhoffa
requested changes
Feb 27, 2025
Collaborator
jdhoffa
left a comment
jdhoffa
left a comment
There was a problem hiding this comment.
A few minor changes, then looks good!
jdhoffa
approved these changes
Feb 27, 2025
jdhoffa
reviewed
Feb 27, 2025
jdhoffa
reviewed
Feb 27, 2025
jdhoffa
reviewed
Feb 27, 2025
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.
Changed services to validate each row when reading in csv against mtcar model. Updated routers as list of mtcars now being passed to the router rather than a json object (fast api will encode the list into json). Created new test to validate that each item in the list at api/dataset is valid mtcar item.