Skip to content

refactor(optimiser): Enhance module slot merging and filtering logic - #4251

Merged
leslieyip02 merged 1 commit into
nusmodifications:masterfrom
thejus03:fix/modules-with-multiple-slots
Nov 10, 2025
Merged

refactor(optimiser): Enhance module slot merging and filtering logic#4251
leslieyip02 merged 1 commit into
nusmodifications:masterfrom
thejus03:fix/modules-with-multiple-slots

Conversation

@thejus03

Copy link
Copy Markdown
Contributor

Context

Fixes #4249

Implementation

Explained in some detail in #4250

- Introduced sorting of module slots by Day and StartTime to ensure consistent combination keys.
- Updated the combination key generation to handle cases with non-E-Venues, preventing duplicates.
- Improved parsing of module slot fields before processing to handle invalid slots more effectively.
@vercel

vercel Bot commented Oct 31, 2025

Copy link
Copy Markdown

@thejus03 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

codecov Bot commented Oct 31, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.13%. Comparing base (988c6fd) to head (c15760a).
⚠️ Report is 148 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4251      +/-   ##
==========================================
+ Coverage   54.52%   55.13%   +0.60%     
==========================================
  Files         274      297      +23     
  Lines        6076     6724     +648     
  Branches     1455     1617     +162     
==========================================
+ Hits         3313     3707     +394     
- Misses       2763     3017     +254     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zehata

zehata commented Nov 6, 2025

Copy link
Copy Markdown
Contributor

Sorry, was busy with something else, let me try this out locally.

@zehata

zehata commented Nov 7, 2025

Copy link
Copy Markdown
Contributor

@thejus03 Thanks for the PR, I have tried it locally, it fixed the issue relating to collision, and I have found that it has no conflict with #4225 and has also fixed the issue with some slots not being serialized, so it looks good to me 👍

cc: @leslieyip02

@vercel

vercel Bot commented Nov 10, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
nusmods-export Ready Ready Preview Comment Nov 10, 2025 7:24pm
nusmods-website Ready Ready Preview Comment Nov 10, 2025 7:24pm

@leslieyip02 leslieyip02 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @thejus03 for the fix and @zehata for the feedback!

@leslieyip02
leslieyip02 merged commit c1a639d into nusmodifications:master Nov 10, 2025
6 checks passed
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.

Optimiser Result Lesson Collision

3 participants