-
Notifications
You must be signed in to change notification settings - Fork 5
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 sales find feature #136
Conversation
updated version
… into branch-v1.3-DG-WangQian * 'branch-v1.3-DG-WangQian' of https://github.com/Persdre/tp: Update StorageClassDiagram.png Update JsonIngredientBookStorageTest.java Updated DG Update JsonIngredientBookStorageTest.java Update SetCommand.java Update diagram Update to pass CheckStyle Add more tests Add tests Implement Ingredient Storage Create IngredientListSequenceDiagram.puml Implement ingredient storage # Conflicts: # src/main/java/seedu/address/model/Model.java # src/main/java/seedu/address/model/UserPrefs.java
update it with latest version
Branch v1.3 dg wang qian
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
============================================
+ Coverage 62.17% 62.27% +0.10%
- Complexity 601 608 +7
============================================
Files 111 115 +4
Lines 2176 2203 +27
Branches 262 265 +3
============================================
+ Hits 1353 1372 +19
- Misses 733 738 +5
- Partials 90 93 +3
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.
Implementation part is LGTM. I have generally left comments about cleaning up code and some suggestions about the UG/DG.
docs/UserGuide.md
Outdated
Format: `s-find KEYWORD [MORE_KEYWORDS]` | ||
|
||
* The search is case-insensitive. e.g `bsbbt` will match `BSBBT`. | ||
* Only the drink's short form name is searched. |
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.
Instead of saying short form name, abbreviation would be better.
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.
Thanks! Changing now:)
docs/UserGuide.md
Outdated
* The search is case-insensitive. e.g `bsbbt` will match `BSBBT`. | ||
* Only the drink's short form name is searched. | ||
* Only full words will be matched e.g. `BSB` will not match `BSBBT`. | ||
* Drinks matching at least one keyword will be returned (i.e. `OR` search). |
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.
Saying OR
search may not be that user-friendly?
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.
en, yes, I agree. I understand but maybe users' can't. I will give a concrete example in the bracket. Thanks~
docs/DeveloperGuide.md
Outdated
|
||
##### Aspect: How to find drink's sales data | ||
|
||
* **Alternative 1 (current choice):** Obtain the drink's name entered by the user, and use the |
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.
Since you only stated one implementation, calling it Alternative 1
may not be that good? Since there was no other alternatives considered.
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.
hh ok I copied the format from original UG. I will delete alternative one here.
// @Test | ||
// public void execute_multipleKeywords_multipleDrinksFound() { | ||
// String expectedMessage = String.format(MESSAGE_DRINKS_LISTED_OVERVIEW, 3); | ||
// InputContainsKeywordsPredicate predicate = preparePredicate("BSBMT BSPM BSBBT"); | ||
// SalesFindCommand command = new SalesFindCommand(predicate); | ||
// expectedModel.updateFilteredSalesList(predicate); | ||
// assertCommandSuccess(command, model, expectedMessage, expectedModel); | ||
// assertEquals(Arrays.asList(), model.getFilteredSalesRecordList()); | ||
// } | ||
|
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.
Remember to delete this test if unused.
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.
thanks
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
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
Update with the latest version
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
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
No description provided.