-
Notifications
You must be signed in to change notification settings - Fork 74
Create and use generic getQuery across section, course, and professor… #290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Great! Will review them soon! |
|
@razeenr05 I left some comments on your PR. Please check it and reach out to me if you have any questions. |
Okay, should I do these changes and create another pull request? |
|
@razeenr05 , you just need to commit new changes, and it will auto-update the PR :)) |
|
Could you check my new commits? |
|
@razeenr05, I will soon! Thank you. But before that, can you resolve the merge conflicts you have? The best way to resolve merge conflicts is pulling new changes from repo to your local and then manually resolving them. I think in your case, you would have to make some further changes to course endpoints. After that, you can commit your merge and push back. |
Okay, I believe I've done so. Hopefully none of the changes I made before were lost lol. Let me know if the merge conflicts were resolved, and the next steps for my solution! |
|
@razeenr05 , great progress so far. Just a few things to fix, what does do? I don't think it's needed for reasons I've included in the review. After resolving all these, this should be ready for merging. |
Oh it was meant to return just one item for the byid case and a 404 if nothing was found. But yea you’re right since a professor can teach multiple courses it makes more sense to just return the full list. I'll do these changes you listed real quick |
|
Hey @razeenr05 , your changes are functional, now 👍, just a few more things I added. Sorry for bugging you like this, I plan on merging your PR before everything else since it should've been merged a while ago but I was just too busy with my school (I still am lol). |
Alright thanks for the help, I believe ive done those last few changes. let me know! |
Please let me know if my code for this issue is okay!
Issue: Create the generic version of query builder function #278