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 view command #111

Merged
merged 6 commits into from
Oct 11, 2020
Merged

Conversation

tohyuting
Copy link

View Command has been implemented, and list command as been edited.
Also amended the first entry of json file/typical warehouses since a match by first word of name in list test would result in finding for the word (warehouse), which would cause all 7 entries to be found by the predicate function.

Edited User Guide to include list command, have also updated the developer guide for list command's use case.

@codecov-io
Copy link

codecov-io commented Oct 11, 2020

Codecov Report

Merging #111 into master will increase coverage by 0.53%.
The diff coverage is 94.87%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #111      +/-   ##
============================================
+ Coverage     69.45%   69.99%   +0.53%     
- Complexity      538      551      +13     
============================================
  Files            87       89       +2     
  Lines          1814     1853      +39     
  Branches        211      217       +6     
============================================
+ Hits           1260     1297      +37     
- Misses          507      508       +1     
- Partials         47       48       +1     
Impacted Files Coverage Δ Complexity Δ
...n/java/seedu/clinic/logic/parser/ClinicParser.java 73.68% <0.00%> (-4.10%) 10.00 <0.00> (ø)
.../java/seedu/clinic/logic/commands/ViewCommand.java 95.45% <95.45%> (ø) 8.00 <8.00> (?)
.../java/seedu/clinic/logic/commands/ListCommand.java 100.00% <100.00%> (ø) 2.00 <0.00> (ø)
...a/seedu/clinic/logic/parser/ViewCommandParser.java 100.00% <100.00%> (ø) 5.00 <5.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c11250...941fb95. Read the comment docs.

Copy link

@qlchan24 qlchan24 left a comment

Choose a reason for hiding this comment

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

Apart from minor comments, LGTM

import seedu.clinic.model.attribute.NameContainsKeywordsPredicateForSupplier;
import seedu.clinic.model.attribute.NameContainsKeywordsPredicateForWarehouse;

public class ViewCommand extends Command {

Choose a reason for hiding this comment

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

To do javadocs

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, missed out on the javadocs comment for class!

@@ -2,7 +2,7 @@
"_comment": "Clinic save file which contains the same supplier values as in TypicalSuppliers#getTypicalAddressBook()",

Choose a reason for hiding this comment

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

comment should be updated as well

Copy link
Author

Choose a reason for hiding this comment

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

Noted, will updated in next commit as well.

@@ -32,8 +34,14 @@ public void execute_listIsNotFiltered_showsSameList() {
}

@Test
public void execute_listIsFiltered_showsEverything() {
public void execute_listIsFilteredSupplier_showsEverythingWarehouseShowOneSupplier() {

Choose a reason for hiding this comment

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

i think the naming for this is a bit confusing, at least at first glance, perhaps can change everything to all


@Test
public void parse_correctTypeCorrectKeyWords_success() {
ViewCommand expectedViewCommand1 = new ViewCommand("supplier", Arrays.asList(new String[]

Choose a reason for hiding this comment

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

I think you can just use list.of() instead of arrays.aslist(String[]{})

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, let me try to use that instead of arrays.aslist(String[]{})

@@ -80,6 +81,9 @@ public static Clinic getTypicalClinic() {
for (Supplier supplier : getTypicalSuppliers()) {
ab.addSupplier(supplier);

Choose a reason for hiding this comment

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

ab used to stand for addressbook, now it has no meaning so i think it should be changed

@tohyuting tohyuting added this to the v1.2 milestone Oct 11, 2020
Copy link

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

@jeffreytjs jeffreytjs merged commit 4162055 into AY2021S1-CS2103-W14-4:master Oct 11, 2020
@tohyuting tohyuting self-assigned this Oct 12, 2020
jeffreytjs added a commit to jeffreytjs/CLI-nic that referenced this pull request Oct 27, 2020
jeffreytjs added a commit to jeffreytjs/CLI-nic that referenced this pull request Oct 27, 2020
jeffreytjs added a commit to jeffreytjs/CLI-nic that referenced this pull request Oct 27, 2020
jeffreytjs added a commit to jeffreytjs/CLI-nic that referenced this pull request Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants