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

Add tests to Module and fix bugs of Lesson #301

Merged
merged 6 commits into from
Nov 7, 2020
Merged

Add tests to Module and fix bugs of Lesson #301

merged 6 commits into from
Nov 7, 2020

Conversation

UncleGrandpa925
Copy link

No description provided.

@UncleGrandpa925 UncleGrandpa925 added this to the v1.4 milestone Nov 7, 2020
@UncleGrandpa925
Copy link
Author

Hi @simonteozw I think this PR is good to merge immediately but I just want you to take a look to see what I have done in this PR (mostly for the refactor of Lesson's messages, I wasn't aware that Lesson's messages structure were quite messy)

Copy link

@simonteozw simonteozw left a comment

Choose a reason for hiding this comment

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

Looks good! Just 2 comments

Comment on lines 279 to 283
@Override
public boolean canAddModule() {
return getFilteredModuleList().size() < NUM_MODULE_LIMIT;
}

Choose a reason for hiding this comment

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

I feel that this function should also check if the module alr exists in trackit? Or maybe rename the function to hasEnoughSpace to make it clearer

Comment on lines +18 to +32
//
// /**
// * Updates {@code model}'s filtered list to show only the module at the given {@code targetIndex} in the
// * {@code model}'s address book.
// */
// public static void showModuleAtIndex(Model model, Index targetIndex) {
// assertTrue(targetIndex.getZeroBased() < model.getFilteredModuleList().size());
//
// Module module = model.getFilteredModuleList().get(targetIndex.getZeroBased());
// Predicate<Module> p = t -> t.getName().equals(module.getName());
// model.updateFilteredModuleList(p);
//
// assertEquals(1, model.getFilteredModuleList().size());
// }
}

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Author

Choose a reason for hiding this comment

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

Ah this I copied from TaskTestUtil but realised it's not usable yet

Copy link

@simonteozw simonteozw left a comment

Choose a reason for hiding this comment

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

LGTM

@UncleGrandpa925 UncleGrandpa925 merged commit 5fbfe3b into AY2021S1-CS2103T-W13-4:master Nov 7, 2020
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.

None yet

2 participants