-
Notifications
You must be signed in to change notification settings - Fork 6
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 match command #217
Add match command #217
Conversation
keanecjy
commented
Oct 21, 2020
•
edited
edited
- Add match command for matching internships to appear in a pop-up window
- To update the UI For the pop-up window
…into match-command � Conflicts: � src/main/java/seedu/address/model/FilterableItemList.java � src/test/java/seedu/address/logic/parser/EditCommandParserTest.java
Codecov Report
@@ Coverage Diff @@
## master #217 +/- ##
============================================
- Coverage 48.35% 47.16% -1.19%
- Complexity 759 765 +6
============================================
Files 184 187 +3
Lines 3464 3547 +83
Branches 389 394 +5
============================================
- Hits 1675 1673 -2
- Misses 1689 1772 +83
- Partials 100 102 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, a few comments added.
src/main/java/seedu/address/logic/commands/util/CommandUtil.java
Outdated
Show resolved
Hide resolved
|
||
public static final String MESSAGE_CONSTRAINTS = "Requirements should not be blank"; | ||
|
||
public static final String MESSAGE_CONSTRAINTS = "Requirement have a maximum of 2 words and not be blank"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i missed why is it limited to 2 words in particular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were discussing this previously and we decided that this allows an easier skill matching.
Because the user can key in any type of requirements which can be very long will definitely render the skill matching useless.
public boolean matches(List<String> skillList) { | ||
assert skillList != null; | ||
return requirements.stream().anyMatch(requirement -> | ||
skillList.stream().anyMatch(skill -> skill.contains(requirement.toString()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly I don't think this is a good idea?
suppose the requirement is "c" a common programming language, it will literally match with any word with a letter c (like cooking) or java with javascript, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keanecjy maybe you could parse the skill title to [str1, str2, str3] then check if any of the substr matches your requirements? something to that effect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Will update.
…into match-command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!