nusmodifications / nusmods

🏫 Official course planning platform for National University of Singapore.
https://nusmods.com
MIT License
558 stars 270 forks source link

Update checkPrerequisite to match wildcard prerequisites #3737

Closed ChillinRage closed 1 day ago

ChillinRage commented 4 days ago

Change checking to parse for wildcards and check for matching prefixes instead of exact string matches.

Context

Proposal to fix #3733 where prerequisite wildcards returns zero matches.

Implementation

Changed the checkPrerequisite function in planner.ts to check if any module in the set has a prefix that matches the string before the wildcard symbol (i.e. UTC in UTC%).

The implementation is still the same as before for non-wildcard modules.

Other Information

vercel[bot] commented 4 days ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nusmods-export ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 3, 2024 4:34pm
nusmods-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 3, 2024 4:34pm
vercel[bot] commented 4 days ago

@ChillinRage is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

codecov[bot] commented 1 day ago

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 53.62%. Comparing base (1d4ec40) to head (969dcbe).

Files Patch % Lines
website/src/utils/planner.ts 91.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3737 +/- ## ========================================== + Coverage 53.53% 53.62% +0.09% ========================================== Files 274 274 Lines 6017 6025 +8 Branches 1443 1446 +3 ========================================== + Hits 3221 3231 +10 + Misses 2796 2794 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ChillinRage commented 1 day ago

Hi sorry for the late response. I have fixed the linter errors and warnings already. One question I have is why are iterative functions (like forEach, filter etc.) preferred over for loops?

ravern commented 1 day ago

Thank you for your fixes! And no need to apologise on response time, there's no rush on fixes/features :)

One question I have is why are iterative functions (like forEach, filter etc.) preferred over for loops?

Unfortunately I don't have a good reason for you, since this is just part of the linting configuration we're using — we didn't explicitly opt-in to avoiding for of loops.

There's some discussion at https://github.com/airbnb/javascript/issues/1271 about why this rule exists, but I personally don't think there's a lot of productive discussion and it's really more of an opinion.

I would advise to just follow the linting configuration for now, unless it is obviously wrong/broken, in which case you can raise it up in your PR description for your reviewers.

ravern commented 1 day ago

You'll see your change in https://latest.nusmods.com, and we'll update once it's on https://nusmods.com (when we merge to production branch). Congrats on your first contribution @ChillinRage!