Gabriel Guirguis - GS Onboarding#21
Conversation
Yarik-Popov
left a comment
Yarik-Popov
left a comment
There was a problem hiding this comment.
Great you are back. The backend is on the right track. There are some changes that are needed to improve it.
backend/api/endpoints/command.py
Outdated
| query = select(Command).where(Command.id == id) | ||
| result = db.exec(query).first() | ||
|
|
||
| if not result: |
| response = await call_next(request) | ||
|
|
||
| duration = perf_counter() - start | ||
| logger.info |
There was a problem hiding this comment.
Good you are using perf_counter but the logging portion isnt done yet
backend/data/data_models.py
Outdated
| validated_params = self.params | ||
| validated_format = self.format | ||
|
|
||
| if validated_params == None and validated_format == None: |
There was a problem hiding this comment.
Use is instead of == when doing comparison against None
backend/data/data_models.py
Outdated
|
|
||
| if validated_params == None and validated_format == None: | ||
| return self | ||
| elif isinstance(validated_params, str) and (isinstance(validated_format, str)) and validated_params.count(",") == validated_format.count(","): |
There was a problem hiding this comment.
Good you are checking if they are strings here. But .count() doesnt produce the correct result. For example "a,b" and "a," have the same number of commas but not the same number of comma separated values (empty isnt considered valid here)
There was a problem hiding this comment.
Remove the redundant () around the 2nd isinstance
frontend/src/display/command_api.ts
Outdated
| await axios.delete(`${API_URL}/${id}`) | ||
| const { data } = await axios.get<CommandListResponse>(`${API_URL}/commands/`) |
There was a problem hiding this comment.
Delete returns all the data so the get call is redundant and slows down performance
backend/data/data_models.py
Outdated
| validated_params = self.params | ||
| validated_format = self.format |
There was a problem hiding this comment.
Kinda of redundant ngl, the right side is shorter and more explicit
backend/data/data_models.py
Outdated
| params: {validated_params} | ||
| format: {validated_format}\n""")) | ||
|
|
||
| return self |
There was a problem hiding this comment.
This return will never be reached because of the else raise block above
backend/data/data_models.py
Outdated
| else: | ||
| raise(ValueError | ||
| (f"""params and format must both be either None or have the same number of comma seperated values, | ||
| recieved: | ||
| params: {validated_params} | ||
| format: {validated_format}\n""")) |
There was a problem hiding this comment.
You can remove the else and unindent the raise. Also remove the () after the raise
|
I didnt review anything for the frontend past the api endpoint file |
|
One thing to note for later. When you start working on a task do not create a fork on the main repo but clone it and branch off to work on a task. This will save u time needing to fix issues later |
2 participants
Purpose
Completed the GS on-boarding task. Include a screenshot of the front-end of the application.
New Changes
Testing
Outstanding Changes