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

Fix UG and DG issues #279

Merged
merged 13 commits into from
Nov 10, 2024
Merged

Conversation

travisim
Copy link

@travisim travisim commented Nov 10, 2024

Fixes #197, Fixes #198, Fixes #199, Fixes #206, Fixes #209, Fixes #223, Fixes #229, Fixes #231, Fixes #233, Fixes #234, Fixes #235, Fixes #237, Fixes #238, Fixes #244, Fixes #253, Fixes #258, Fixes #268, Fixes #269

Copy link

codecov bot commented Nov 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Copy link

@chongtzezhao chongtzezhao left a comment

Choose a reason for hiding this comment

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

LGTM. Good job Travis!

<box type="tip" seamless>

The field PUBLIC_ADDRESS is not cap sensitive.
This command only searches the current list of public addresses displayed(eg if all contacts is displayed

Choose a reason for hiding this comment

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

nit: (grammar) change "if all contacts is displayed" to "if all contacts are displayed"

Copy link
Author

Choose a reason for hiding this comment

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

yes sir this shall be fixed

Comment on lines 441 to 444
add to future enhancements
The command 'forgiving' these extraneous inputs (i.e., giving an output same as or similar to if those inputs are not
present) is not incorrect. You can mention in the UG that such inputs will be ignored. AB3 already does a similar thing
for some commands. Any special handling of such inputs can be left as a future enhancement.

Choose a reason for hiding this comment

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

Is this meant to be here?

Copy link
Author

Choose a reason for hiding this comment

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

no its supposed to be in UG

@@ -192,7 +216,7 @@ DLTbook comes with sample data to help you get started. Here are some basic comm
3. **Add a DLT public address**

```
addpa 1 c/BTC l/wallet1 pa/0x28f91d6e72eaf4372892e6c6e45dc41b574163e9fcdf94f4997958b46d772fa2
addpa c/ETH n/Travis w/wallet1 pa/0x0b1c9e1fb5e13c797c7f0134641810e9a7ca14d2

Choose a reason for hiding this comment

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

Isn't this the wrong format?

Copy link
Author

Choose a reason for hiding this comment

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

apologies again


<box type="tip" seamless>

The field PUBLIC_ADDRESS is not cap sensitive.

Choose a reason for hiding this comment

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

Standardise to case sensitive? Or doesn't matter

Copy link
Author

Choose a reason for hiding this comment

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

apologies, I shall use case-insensitive

@@ -674,9 +733,9 @@ the data of your previous AddressBook home folder.
| **List** | `list` |
| **Help** | `help` |
| **Exit** | `exit` |
| **Add Public Address** | `addpa INDEX c/NETWORK l/WALLET_NAME pa/PUBLIC_ADDRESS`<br> e.g., `addpa 1 c/ETH l/wallet1 pa/0x28f91d6e72eaf4372892e6c6e45dc41b574163e9fcdf94f4997958b46d772fa2` |
| **Edit Public Address** | `editpa INDEX c/NETWORK l/WALLET_NAME pa/NEW_ADDRESS`<br> e.g., `editpa 3 c/BTC l/Daily wallet pa/14qViLJfdGaP4EeHnDyJbEGQysnCpwk3gd` |
| **Add Public Address** | `addpa c/NETWORK n/NAME w/WALLET_NAME pa/PUBLIC_ADDRESS`<br> e.g., `addpa c/ETH n/Travis w/wallet1 pa/0x0b1c9e1fb5e13c797c7f0134641810e9a7ca14d2` |

Choose a reason for hiding this comment

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

Wrong format again?

Copy link
Author

Choose a reason for hiding this comment

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

yes, fixed

| **Retrieve Public Address** | `retrievepa l/WALLET_NAME [c/NETWORK] [n/PERSON_NAME]`<br> e.g., `retrievepa l/wallet1 c/BTC n/John` |
| **Delete Public Address** | `deletepa c/NETWORK [l/WALLET_NAME]`<br> e.g., `deletepa 1 c/BTC l/wallet1` |
| **Public Address Search** | `searchpa pa/PUBLIC_ADDRESS`<br> e.g., `searchpa pa/0x28f91d6e72eaf4372892e6c6e45dc41b574163e9fcdf94f4997958b46d772fa2` |
| **Delete Public Address** | `deletepa c/NETWORK [w/WALLET_NAME]`<br> e.g., `deletepa 1 c/BTC w/wallet1` |

Choose a reason for hiding this comment

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

Wrong field

Copy link
Author

Choose a reason for hiding this comment

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

fixed this

Copy link

@Nicholascyx Nicholascyx left a comment

Choose a reason for hiding this comment

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

LGTM, all public addresses are now correct examples to be used.

@@ -674,9 +730,9 @@ the data of your previous AddressBook home folder.
| **List** | `list` |
| **Help** | `help` |
| **Exit** | `exit` |
| **Add Public Address** | `addpa INDEX c/NETWORK l/WALLET_NAME pa/PUBLIC_ADDRESS`<br> e.g., `addpa 1 c/ETH l/wallet1 pa/0x28f91d6e72eaf4372892e6c6e45dc41b574163e9fcdf94f4997958b46d772fa2` |
| **Edit Public Address** | `editpa INDEX c/NETWORK l/WALLET_NAME pa/NEW_ADDRESS`<br> e.g., `editpa 3 c/BTC l/Daily wallet pa/14qViLJfdGaP4EeHnDyJbEGQysnCpwk3gd` |
| **Add Public Address** | `addpa INDEX c/NETWORK l/WALLET_NAME pa/PUBLIC_ADDRESS`<br> e.g., `addpa 1 c/ETH l/wallet1 pa/0x0b1c9e1fb5e13c797c7f0134641810e9a7ca14d2` |z

Choose a reason for hiding this comment

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

There is a "z" at the end here.

Copy link
Author

Choose a reason for hiding this comment

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

that is fixed

to the first person on the list `Alex Yeoh` under the ETH network with the wallet name `wallet1` and the public
address `0x28f91d6e72eaf4372892e6c6e45dc41b574163e9fcdf94f4997958b46d772fa2`.
* `addpa 1 c/ETH l/wallet1 pa/0x0b1c9e1fb5e13c797c7f0134641810e9a7ca14d2
* ` adds a public address

Choose a reason for hiding this comment

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

Maybe adds a public address should be connected with the command, instead of being a new point?

Copy link
Author

Choose a reason for hiding this comment

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

good catch

@travisim travisim merged commit b71cbe4 into AY2425S1-CS2103T-T08-1:master Nov 10, 2024
4 checks passed
@travisim travisim added this to the v1.6 milestone Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment