Skip to content
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

refactor, fix: validate trpc & unsorted semesters bug #716

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

kamui-fin
Copy link
Contributor

Overview

Refactored requisite validation logic under validator.ts. In the process, I encountered a major bug where the semesters from the fetched planData was not sorted. The problem was that the requisite validation recursive function was comparing semester indices, which obviously would be broken with unsorted semesters. A fix is included in this PR.

@github-actions
Copy link

Copy link
Member

@cubetastic33 cubetastic33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great! Do you think you can add a bunch of comments? Also see if you can add the group by on the data pulled from API so we show more courses in the search.

@github-actions
Copy link

Copy link
Member

@cubetastic33 cubetastic33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for now because I think the fix is important

@kamui-fin kamui-fin merged commit 7073c4b into develop Oct 24, 2023
6 checks passed
@kamui-fin kamui-fin deleted the refactor-validatets branch October 24, 2023 15:11
cubetastic33 added a commit that referenced this pull request Nov 10, 2023
This reverts commit 7073c4b, reversing
changes made to 15f234c.
cubetastic33 added a commit that referenced this pull request Nov 10, 2023
Revert "Merge pull request #716 from UTDNebula/refactor-validatets"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants