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

Update module error message and 2 suffix allowance #240

Merged

Conversation

whitesnowx
Copy link
Collaborator

@whitesnowx whitesnowx commented Apr 12, 2024

Plausible module: ASP1201CH
--> Increase suffix allowed to increase scope for valid inputs

Closes #235

@whitesnowx whitesnowx added the type.Task Something that needs to be done, but not a story, bug, or an epic. label Apr 12, 2024
@whitesnowx whitesnowx added this to the v1.4 milestone Apr 12, 2024
@whitesnowx whitesnowx self-assigned this Apr 12, 2024
@whitesnowx whitesnowx added the priority.Medium Nice to have label Apr 12, 2024
Copy link
Collaborator

@iynixil iynixil left a comment

Choose a reason for hiding this comment

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

lgtm! minor nits

@@ -11,8 +11,8 @@ public class Module {


public static final String MESSAGE_CONSTRAINTS =
"Module code should contain 2-4 capital letters followed by 4 digits long and at most 1 capitalised suffix";
public static final String VALIDATION_REGEX = "[a-zA-Z]{2,4}\\d{4}[a-zA-Z]{0,1}";
"Module code should contain 2-4 letters followed by 4 digits long and at most 2 capitalised suffix";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be and suffix of at most 2 characters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

amended 🔧

Comment on lines 40 to 45
assertTrue(Module.isValidModule("CS2103")); // 2 prefix, 4 letters without 1 optional suffix
assertTrue(Module.isValidModule("CS2103T")); // 2 prefix, 4 letters with 1 optional suffix
assertTrue(Module.isValidModule("CS2103TT")); // 2 prefix, 4 letters with 2 optional suffix
assertTrue(Module.isValidModule("GEN2050")); // 3 prefix, 4 letters without 1 optional suffix
assertTrue(Module.isValidModule("GEN2050Y")); // 3 prefix, 4 letters with 1 optional suffix
assertTrue(Module.isValidModule("GESS1035")); // 4 prefix, 4 letters without 1 optional suffix
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Misleading comment 4 letters -> 4 digits?

Copy link
Collaborator Author

@whitesnowx whitesnowx Apr 12, 2024

Choose a reason for hiding this comment

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

amended 🔧

@iynixil iynixil merged commit 5b6fee0 into AY2324S2-CS2103-F08-3:master Apr 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.Medium Nice to have type.Task Something that needs to be done, but not a story, bug, or an epic.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Module Validity and error message
2 participants