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

[#162] Show warning when ignoring repeated parameters #176

Conversation

Eclipse-Dominator
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator commented May 17, 2023

Fixes #162

Currently, repeated occurrences of the same parameter in a command are being ignored without any notification to the user. This can lead to potential user errors going unnoticed, especially when using the wrong parameter prefix multiple times.

This commit introduces a friendly warning system that alerts the user when a parameter is being ignored. Now, if a parameter is repeated in a command, all occurrences except the last one will trigger a warning message, ensuring that the user is aware of any unintentional mistakes or typos.

Let's provide a better user experience by catching and notifying them of ignored parameters, improving the overall usability and preventing potential errors.

Repeated occurrences of the same parameter in a command are being
ignored without any notification to the user.
This can lead to potential user errors going unnoticed, especially when
using the wrong parameter prefix multiple times.

Let's introduces a warning system that alerts the user when a
parameter is being ignored.
Now, if a parameter is repeated in a command,
a warning message will be displayed to ensure that
the user is aware of any unintentional mistakes or typos.
@canihasreview
Copy link

canihasreview bot commented May 17, 2023

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

See this repository's contribution guide for more information.

@canihasreview
Copy link

canihasreview bot commented May 17, 2023

v1

@Eclipse-Dominator submitted v1 for review.

(📚 Archive)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/176/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@codecov-commenter
Copy link

Codecov Report

Merging #176 (ce23ddf) into master (1e05fef) will decrease coverage by 0.23%.
The diff coverage is 58.82%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff              @@
##             master     #176      +/-   ##
============================================
- Coverage     72.15%   71.93%   -0.23%     
- Complexity      399      402       +3     
============================================
  Files            70       70              
  Lines          1232     1247      +15     
  Branches        125      131       +6     
============================================
+ Hits            889      897       +8     
- Misses          311      312       +1     
- Partials         32       38       +6     
Impacted Files Coverage Δ
.../seedu/address/logic/parser/EditCommandParser.java 81.25% <33.33%> (-11.06%) ⬇️
...java/seedu/address/logic/commands/EditCommand.java 93.42% <72.72%> (-3.60%) ⬇️

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

@Eclipse-Dominator Eclipse-Dominator changed the title Show warning when ignoring repeated parameters [#162] Show warning when ignoring repeated parameters May 17, 2023
Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

I'm a bit hesitant about this approach because,

  1. It doesn't inform the user which parameter is duplicated
  2. It doesn't differentiate between one parameter being duplicated vs multiple parameters being duplicated

Perhaps we should explore alternatives to see if there is a better way?

@@ -49,6 +49,8 @@ public class EditCommand extends Command {
public static final String MESSAGE_EDIT_PERSON_SUCCESS = "Edited Person: %1$s";
public static final String MESSAGE_NOT_EDITED = "At least one field to edit must be provided.";
public static final String MESSAGE_DUPLICATE_PERSON = "This person already exists in the address book.";
public static final String MESSAGE_DUPLICATE_FIELD = "Detected multiple modification to a same parameter, "
+ "applying the latest change.\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

modification -> modifications?
Is it correct to say modifications i.e., are these strictly modifications or something else? Similarly, 'latest change' doesn't seem like the correct phrase.

@Eclipse-Dominator
Copy link
Contributor Author

Hmm, alright, I chose this initially as I felt this will require the least amount of changes but I will come up with a secondary approach to display which field has been duplicated.

@damithc
Copy link
Contributor

damithc commented May 18, 2023

Hmm, alright, I chose this initially as I felt this will require the least amount of changes but I will come up with a secondary approach to display which field has been duplicated.

Yup, this may still be the best option but it's better to explore other options before making a decision.

@Eclipse-Dominator
Copy link
Contributor Author

I have created a secondary PR to show an alternative approach in #181

@Eclipse-Dominator
Copy link
Contributor Author

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

@damithc
Copy link
Contributor

damithc commented Jun 12, 2023

Closing in favor of #190

@damithc damithc closed this Jun 12, 2023
@Eclipse-Dominator Eclipse-Dominator deleted the 162-give-a-warning-when-a-parameter-is-repeated-in-a-user-command branch June 14, 2023 03:29
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.

Give a warning when a parameter is repeated in a user command
3 participants