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

feat: migration for new cs major requirements #939

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

import-brain
Copy link
Member

@import-brain import-brain commented Aug 24, 2024

Summary

Computer Science majors who enter Cornell in Fall 2024 have different requirements compared to those who entered Cornell before that: CS 3700 or CS 3780 is now a required course, there are only two required CS electives, and there is no Probability requirement.

This PR implements migrations in the CS major file to reflect these differences for future CS majors who entered Cornell in Fall 2024.

@import-brain import-brain requested a review from a team as a code owner August 24, 2024 11:20
@CLAassistant
Copy link

CLAassistant commented Aug 24, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@import-brain import-brain changed the title feat: migration for new cs core and elective requirements feat: migration for new cs major requirements Aug 24, 2024
@plumshum
Copy link
Contributor

Summary

Computer Science majors who enter Cornell in Fall 2024 have different requirements compared to those who entered Cornell before that: CS 3700 or CS 3780 is now a required course, there are only two required CS electives, and there is no Probability requirement.

This PR implements migrations in the CS major file to reflect these differences for future CS majors who entered Cornell in Fall 2024.

Summary

Computer Science majors who enter Cornell in Fall 2024 have different requirements compared to those who entered Cornell before that: CS 3700 or CS 3780 is now a required course, there are only two required CS electives, and there is no Probability requirement.

This PR implements migrations in the CS major file to reflect these differences for future CS majors who entered Cornell in Fall 2024.

hiiiiiii thanks for the pr request Eric!

Copy link
Member

@Destaq Destaq left a comment

Choose a reason for hiding this comment

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

Well done Eric, amazing that you are already contributing to CoursePlan like this before freshman even join the team! There are a couple minor issues with your PR — see my comments — but fantastic work. Hopefully you will see your new CS requirements in production on courseplan.io soon 👍 .

src/data/majors/cs.ts Show resolved Hide resolved
src/data/majors/cs.ts Outdated Show resolved Hide resolved
@@ -25,7 +25,7 @@ export type typeOfMigration = 'Modify' | 'Delete' | 'Add';

export type RequirementMigration = {
entryYear: number /** This migration applies to students with an entryYear equal to or EARLIER this entry year */;
type: typeOfMigration /** Modify or Delete Migration? This field must already exist in requirements file */;
type: typeOfMigration /** Modify, Delete, or Add Migration? This field must already exist in requirements file */;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch here with the enum update. Honestly maybe we should just get rid of this since duplication is evil and TypeScript serves the purpose of that part of the comment. But this is a commenting style nitpick that was already here before you, just sharing here for reference for our TPM.

src/data/majors/cs.ts Show resolved Hide resolved
src/data/majors/cs.ts Show resolved Hide resolved
src/requirements/decorated-requirements.json Outdated Show resolved Hide resolved
@import-brain
Copy link
Member Author

import-brain commented Oct 23, 2024

Well done Eric, amazing that you are already contributing to CoursePlan like this before freshman even join the team! There are a couple minor issues with your PR — see my comments — but fantastic work. Hopefully you will see your new CS requirements in production on courseplan.io soon 👍 .

Hi Simon, thank you for your in-depth code review! I just added a commit addressing your review comments :).

maintain consistent string styling with rest of codebase
@Destaq
Copy link
Member

Destaq commented Oct 24, 2024

Thank you for the speedy fixes!

@Destaq
Copy link
Member

Destaq commented Nov 17, 2024

@import-brain feel free to merge this into main! Might need to push another commit to the check to run though, or speak to @nidhi-mylavarapu about what's going on here.

@import-brain
Copy link
Member Author

Screenshot 2024-11-18 at 3 04 55 AM

$(git status --porcelain | wc -l) evaluates to 0 on my local machine, not sure why it returns 1 here...

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.

4 participants