Alexiane genealogy#325
Alexiane genealogy#325alexianelaude wants to merge 4 commits intoMines-Paristech-Students:masterfrom
Conversation
alexianelaude
force-pushed
the
alexiane-genealogy
branch
from
7db23d6 to
3531c03
Compare
abocquet
left a comment
abocquet
left a comment
There was a problem hiding this comment.
Thank you for this addition, it will be useful!
This PR is a bit long, generally we try to keep them under 150 lines (sometimes more if there is UI). To have more precise feedback, could you split your PR please? Suggestion:
1/ introducing the AST in the model + basic view (profile, ...)
2/ declare the /genealogy views in the backend with the more advanced functionnalities
3/ add the frontend
I'll give you feedback ASAP each time so you can move forward.
We try to keep the whole repo in english and avoid "frenglish", as it leads to confusion. In this regard, ast_cousin would be fit than 'cousinast'. For "minesparent" it's a pun with the "godparent" word.
In order to make the pipelines pass, you should reformat your code with "black" (https://github.com/psf/black), you can either reformat the whole codebase with it at once or reformat files as you save them (there is a PyCharm extension). This is important so there never are changes linked to formatting in PR reviews, which is very noisy for nothing.
You did a great job, the few comments should be quick to fix 👍
| result = [] | ||
| recherche = False | ||
| result_string = "" | ||
| return Response({'eleves': eleves, 'result': result, 'result_string': result_string, 'recherche': recherche}) |
There was a problem hiding this comment.
we probably want to avoid returning the list of all students at once, especially if there is search in the request
| result_string = "" | ||
| return Response({'eleves': eleves, 'result': result, 'result_string': result_string, 'recherche': recherche}) | ||
|
|
||
| def post(self, request): |
There was a problem hiding this comment.
If it gives an information and does not modify anything, if should be a GETmethod. In api/users.ts, having getGenealogy calling api.post is a bit weird. Instead you can simply declare a new path with a direct method, and you'll spare the dedicated APIView
| return set(Role.objects.filter(query).values_list("association__pk", flat=True)) | ||
|
|
||
| def get_absolute_url(self): | ||
| return '/profils/'+self.id |
There was a problem hiding this comment.
I don't see this function called anywhere
| @staticmethod | ||
| def BFS(start, end): | ||
| """Breadth-First-Search algorithm, to find the shortest path between two students""" | ||
| visited = [] |
There was a problem hiding this comment.
using a set will provide O(log n) complexity vs O(n) with a plain array in the in in line 242
alexianelaude
force-pushed
the
alexiane-genealogy
branch
from
7a289d0 to
dab0d5c
Compare
alexianelaude
force-pushed
the
alexiane-genealogy
branch
from
93f9723 to
992de65
Compare
2 participants
TODO: make this display prettier? (with an actual tree) + should we include the roommate relationship?