harbingerofme / DebugToolkit

Debugging commands for Risk of Rain 2. Previously known as RoR2Cheats.
https://thunderstore.io/package/IHarbHD/DebugToolkit/
BSD 3-Clause "New" or "Revised" License
14 stars 8 forks source link

Change command: create_pickup #139

Closed SChinchi closed 11 months ago

SChinchi commented 1 year ago
harbingerofme commented 1 year ago

Great work!

I'm worried we might break workflows, any chance we can add backwards compatibility for "both" instead of "all". I don't think adding it for coins is sensible with potential for conflicts on that one.

SChinchi commented 1 year ago

I think I see your point. "all" is actually "both" as in only "equip" and "item" for a search term. I used "all" because we already had that keyword, but I now see the confusion it creates. The reason why I added an encompassing term to begin with is so that a user can have the option to use the default value either with "" or an explicit arg. I will change it back.

I assume that also addresses your worry about the coins and that you aren't actually against the command creating coins.

harbingerofme commented 1 year ago

I think I see your point. "all" is actually "both" as in only "equip" and "item" for a search term. I used "all" because we already had that keyword, but I now see the confusion it creates. The reason why I added an encompassing term to begin with is so that a user can have the option to use the default value either with "" or an explicit arg. I will change it back.

I agree with the change to "all", I just want to maintain the backwards compatibility for using "both".

I assume that also addresses your worry about the coins and that you aren't actually against the command creating coins.

There's no concern with coins. The term is outdated since sotv.

SChinchi commented 1 year ago

The signature of the command had always been {object} [search ('item'|'equip'):<both>] [target]. "both" had never been a valid input for the user according to the docs. However, in the past it technically worked, as "b0th"/"zxcv"/"whatever" also worked because they silently defaulted to the default behaviour. But after adding more explicit argument parsing errors, it reported anything not "item" or "equip" as invalid. With proposing [search ('item'|'equip'|'both'):'both'] the functionality is enhanced by making "both" a valid option and it doesn't break any backwards compabitilty. I initially chose "all" for that purpose, but "both" seems to be better.

I hope I haven't misunderstood you.