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

Existing requirement computation issues #138

Closed
SamChou19815 opened this issue Sep 16, 2020 · 1 comment · Fixed by #265
Closed

Existing requirement computation issues #138

SamChou19815 opened this issue Sep 16, 2020 · 1 comment · Fixed by #265

Comments

@SamChou19815
Copy link
Contributor

SamChou19815 commented Sep 16, 2020

Some of my opinions on how we can make the existing requirement computation more maintainable. None of the issues below should block merging a PR.

Types of operator ('and' | 'or' | null) is very confusing.

They don't seem to relate to double counting. A better name would be requiredCourseCheckerStrategy: 'all-of' | 'any-of'.

Bloated BaseRequirement

Instead of combining credits | courses | self-check requirements into a single type. We should use TypeScript's disjointed union capability to split them into 3 different types, and do pattern matching on them. In this way, we can get rid of a bunch of null and undefined possibilibity that are just added for self-check types of courses.

Requirement with several fulfillment strategies

Related to the above issue, we probably should create a new type for requirement like

type RequirementWithMultipleFulfillmentStrategy = {
  readonly type: 'RequirementWithMultipleFulfillmentStrategy'
  readonly strategies: readonly Requirement[]
}

type Requirement = 
  | RequirementToBeFulfilledByCourses
  | RequirementToBeFulfilledByCredits
  | RequirementForSelfCheck
  | RequirementWithMultipleFulfillmentStrategy
@willespencer
Copy link
Member

These all look like good improvements! I'll add them to Monday

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 a pull request may close this issue.

2 participants