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

change a bunch of stuff to be multiple use #1807

Merged
merged 5 commits into from
Jul 29, 2023

Conversation

Malibu-Stacey
Copy link
Contributor

changed all avatar potions to be avatar, multiple and changed a bunch of stuff I found reported in my session logs over the past year to multiple.

@Shiverwarp reported the avatar potions on https://kolmafia.us/threads/multi-usable-items.29006/ which I haven't personally tested or seen in a log as I haven't used any of them (it's mostly people in 2CRS consuming these) but I trust this is correct from Veracity's discussion on that thread.

@Malibu-Stacey Malibu-Stacey requested a review from a team as a code owner July 5, 2023 19:13
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #1807 (8dfa4fd) into main (5e6e8be) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1807      +/-   ##
============================================
- Coverage     36.33%   36.33%   -0.01%     
- Complexity    18762    18764       +2     
============================================
  Files          1081     1081              
  Lines        166346   166345       -1     
  Branches      35394    35394              
============================================
- Hits          60437    60435       -2     
- Misses        96036    96038       +2     
+ Partials       9873     9872       -1     
Files Changed Coverage Δ
...t/sourceforge/kolmafia/request/UseItemRequest.java 16.82% <100.00%> (-0.15%) ⬇️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e6e8be...8dfa4fd. Read the comment docs.

@Veracity0
Copy link
Contributor

This will work.

Thank you for fixing some potions from usable to multiple - based on your own observations.
I could have sworn that bitter pills were not multiusable at first, but perhaps that changed.
Or perhaps I misremember. :)

My comment on Shiverwarp's bug report left open an alternative solution: since ItemDatabase.isMultiUsable already assumes that type "avatar" is that (unless marked otherwise), perhaps UseItemRequest should use that existing method, rather than requiring that the "multiple" flag be set for all such potions in items.txt?

I am not sure which is better.

As I said, your solution works, but I want to think on it a bit more.

@Veracity0
Copy link
Contributor

It is long time since I responded to this. I'm on my phone, so can't call up code, but my response to the kolmafia.us bug report points to exactly the code I'd modify.

The issue is this: given an itemId, UseItemRequest wants to decide if the item is multi usable. It looks only at Attributes: usable, multiple, reusable.

ItemDatabase has methods: isUsable, isMultiUsable, and isReusable (or something). Those methods look at item types and also attributes. A potion or avatar is multi usable UNLESS it is specifically tagged "usable".

  1. All your observations that certain potions are "multiple", not "usable"? Cool. Mark them "multiple". And see step 3. 😀
  2. Fix the UseItemRequest method to call the ItemDatabase methods rather than looking at attributes.
  3. All those potions and avatar potions marked "multiple"? Remove that, since that is the default for a potion or avatar potion.

End result:

In items.txt, every potion is simply "potion" (which implies multiple) or "potion, usable".
And ditto for "avatar". Which probable are always exactly that and never "avatar, usable"

@gausie
Copy link
Contributor

gausie commented Jul 19, 2023

Perhaps we should add a data file integrity test that enforces this? So you can't have multi usable on something also marked as a potion?

@jaadams5 jaadams5 marked this pull request as draft July 29, 2023 12:11
@jaadams5
Copy link
Contributor

Changed to draft because that is a filter I use. There are good ideas here but no activity in at least a week.

@Veracity0
Copy link
Contributor

Veracity0 commented Jul 29, 2023

OK, since I really want this PR, I did all of the suggestions:

  1. UseItemRequest.getConsumptionType now uses ItemDatabase.isReusable, isMultiUsable, and isUsable rather than looking at attributes.
  2. All "potion" and "avatar" items that explicitly were marked "multiple" have that attribute removed.
  3. DataFIleConsistencyTest now verifies that no "potion" or "avatar" item has an explicit "multiple" attribute.

This should be good to go.

@Veracity0 Veracity0 marked this pull request as ready for review July 29, 2023 15:57
@Veracity0 Veracity0 merged commit 61f6ccc into kolmafia:main Jul 29, 2023
5 checks passed
@Malibu-Stacey Malibu-Stacey deleted the mooltiuse branch December 4, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants