search

se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
28 stars 426 forks source link

Removal of repeatable parameters #190

Closed Eclipse-Dominator closed 1 year ago

Eclipse-Dominator commented 1 year ago

This PR an alternative that Fixes #162 (other alternatives: #176, #181)

In this PR, we treat repeated parameters as an error.

If a single-valued parameter is repeated multiple times in a 
command, only the last occurrence is taken.  This allows the user
to correct a mistake in a parameter value by simply typing it again,
which may be faster than moving the cursor back to the original
parameter values and editing it. This feature serves as a good
example of 'going the extra mile to optimize the application for
target users' (i.e., users who can type fast).
However, this feature can cause certain user errors to go unnoticed
e.g., unintended repetition caused by a typo in parameter name

Alternatives considered:
(a) Retain current behavior but give a friendly warning to the user
    that a parameter was 'overridden'. 
(b) Remove this feature i.e., treat repeated single-valued
    parameters    as an error, and reject the command with an
    error message.

Alternative (a) requires significantly more complicated code
(see #181), steepening the learning curve for for new programmers.
Given that this application is primarily meant for training
novice programmers, the impact on the learning curve is not worth
the benefit of retaining this feature.

Therefore, let's do option (b).
canihasreview[bot] commented 1 year ago

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

codecov[bot] commented 1 year ago

Codecov Report

Merging #190 (4a4144d) into master (bd48d59) will increase coverage by 0.16%. The diff coverage is 91.66%.

@@             Coverage Diff              @@
##             master     #190      +/-   ##
============================================
+ Coverage     74.00%   74.16%   +0.16%     
- Complexity      420      428       +8     
============================================
  Files            71       71              
  Lines          1281     1293      +12     
  Branches        126      127       +1     
============================================
+ Hits            948      959      +11     
+ Misses          301      300       -1     
- Partials         32       34       +2
Impacted Files Coverage Δ
src/main/java/seedu/address/logic/Messages.java 87.50% <75.00%> (-4.17%) :arrow_down:
...a/seedu/address/logic/parser/AddCommandParser.java 100.00% <100.00%> (ø)
...a/seedu/address/logic/parser/ArgumentMultimap.java 100.00% <100.00%> (ø)
.../seedu/address/logic/parser/EditCommandParser.java 92.59% <100.00%> (+0.28%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

canihasreview[bot] commented 1 year ago

v1

@Eclipse-Dominator submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v1

@Eclipse-Dominator submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 1 year ago

@Eclipse-Dominator Now that we have three alternatives, what your thoughts about selecting one? Also, in each of the three PR descriptions, you can mention the other two so that anyone visiting the PR can see how they fit into the big picture.

damithc commented 1 year ago

Repeated Parameters show warning: #176 Repeated Parameters show warning of repeated prefixes: #181 Repeated Parametes treated as errors: #190

Updated the PR description instead.

damithc commented 1 year ago

@Eclipse-Dominator As discussed, let's proceed with this approach.

canihasreview[bot] commented 1 year ago

v2

@Eclipse-Dominator submitted v2 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/2/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v3

@Eclipse-Dominator submitted v3 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/3/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v4

@Eclipse-Dominator submitted v4 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v3 and v4) (:chart_with_upwards_trend: Range-Diff between v3 and v4)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/4/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v5

@Eclipse-Dominator submitted v5 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v4 and v5) (:chart_with_upwards_trend: Range-Diff between v4 and v5)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/5/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v6

@Eclipse-Dominator submitted v6 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v5 and v6) (:chart_with_upwards_trend: Range-Diff between v5 and v6)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/6/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v7

@Eclipse-Dominator submitted v7 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v6 and v7) (:chart_with_upwards_trend: Range-Diff between v6 and v7)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/7/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v8

@Eclipse-Dominator submitted v8 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v7 and v8) (:chart_with_upwards_trend: Range-Diff between v7 and v8)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/8/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v9

@Eclipse-Dominator submitted v9 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v8 and v9) (:chart_with_upwards_trend: Range-Diff between v8 and v9)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/9/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v10

@Eclipse-Dominator submitted v10 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v9 and v10) (:chart_with_upwards_trend: Range-Diff between v9 and v10)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/10/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
Eclipse-Dominator commented 1 year ago

Resolve rebase conflicts.

damithc commented 1 year ago

@Eclipse-Dominator Looks like the rebase wasn't entirely correct. There are some unrelated code in this PR.

canihasreview[bot] commented 1 year ago

v11

@Eclipse-Dominator submitted v11 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v10 and v11) (:chart_with_upwards_trend: Range-Diff between v10 and v11)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/11/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v12

@Eclipse-Dominator submitted v12 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v11 and v12) (:chart_with_upwards_trend: Range-Diff between v11 and v12)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/12/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v13

@Eclipse-Dominator submitted v13 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v12 and v13) (:chart_with_upwards_trend: Range-Diff between v12 and v13)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/13/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 1 year ago

@se-edu/tech-team-level1 for your review ...

canihasreview[bot] commented 1 year ago

v14

@Eclipse-Dominator submitted v14 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v13 and v14) (:chart_with_upwards_trend: Range-Diff between v13 and v14)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/14/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v15

@Eclipse-Dominator submitted v15 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v14 and v15) (:chart_with_upwards_trend: Range-Diff between v14 and v15)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/190/15/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.