[Feature] Extend #parcels Command #4351
Conversation
Kinglykrab
changed the title
|
Good evening @Kinglykrab I ran a few tests and would highlight a few items for your consideration. Item #1 - As seen below The Ak'Anon Report Vol 9. No 10. shows a quantity of 0. Should be 1. Item #4 - #parcels 99990 1000 32 does not produce the note. It appears that the note must contain a non-numeric value. #parcels 99990 1000 32s works for example. Item #5 - #parcels add Rola ItemLink 1 note returns the #parcel syntax and does not add the parcel. Command is noted below. Item #6 - #parcels add Rola 6941 1 85490 85490 Note sends the parcel and shows the Fine Steel Jute with the augment in slot 4, and in slot 20 even though the augment is not for those slots. Likely need to do some checks to ensure we don't end up with erroneous items. The resulting item is unusable until the augment is removed. I would suggest some more testing. |
| c->MessageString(Chat::Yellow, CANT_FIND_PLAYER, player_name.c_str()); | ||
| return; | ||
| } | ||
| const std::string& to_name = sep->arg[2]; |
| return; | ||
| } | ||
| if (!sep->IsNumber(3)) { | ||
| const std::string& cmd_msg = sep->msg; |
There was a problem hiding this comment.
Same thing here, though I am not positive. You likely know better than I.
| if (item_id == PARCEL_MONEY_ITEM_ID) { | ||
| if (quantity > INT32_MAX) { | ||
| if (quantity > std::numeric_limits<int>::max()) { |
There was a problem hiding this comment.
Kingly, is there an advantage of using std::numeric_limit over the define in limits.h of INT32_MAX? Curious if there is an advantage from one over the other. Perhaps the compiler replaces them both anyways?
| } | ||
|
|
||
| auto item = database.GetItem(PARCEL_MONEY_ITEM_ID); | ||
| if (!item) { | ||
| c->Message(Chat::Yellow, "Could not find item with id {}", item_id); | ||
| c->Message(Chat::White, "Item ID {} does not exist.", item_id); |
There was a problem hiding this comment.
Are there standards as to which color is for what? Yellow vs white? And the syntax of the messages?
| augment_ids[3], | ||
| augment_ids[4], | ||
| augment_ids[5] | ||
| ); |
There was a problem hiding this comment.
With the change to CreateItem, instead of GetItem, 'item' will need to be deleted at all the right places or it will cause a leak I believe. I would suggest using a unique_ptr so that you don't need to worry about the deletes.
|
|
||
| for (auto const& p: results) { | ||
| const auto* item = database.CreateItem( | ||
| p.item_id, |
There was a problem hiding this comment.
Same delete/unique_ptr concern
|
|
||
| for (auto const& p : parcels) { | ||
| const auto* item = database.CreateItem( | ||
| p.second.item_id, |
There was a problem hiding this comment.
@Kinglykrab Provided a few comments for your consideration. I did not highlight all the string& items as I am not sure it makes a difference. Perhaps someone else can comment on those. Thanks for the chance to review. I can re-test after you have a chance to consider the comments, just let me know.
|
Still has pending feedback |
Description
#parcels addsubcommand.#parcels detailssubcommand.Type of Change
Testing
Checklist