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

Support booze and food fairies #1420

Merged
merged 7 commits into from
Jan 10, 2023
Merged

Conversation

gausie
Copy link
Contributor

@gausie gausie commented Jan 9, 2023

  • Track Vintner and Cookbookbat as food and booze fairies
  • Add Superconductor's CPU as Single Equip while we're here

Note: this adds new familiar types: item1 and item2

@gausie gausie requested a review from a team as a code owner January 9, 2023 17:47
@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Merging #1420 (3141742) into main (3f64c71) will increase coverage by 0.00%.
The diff coverage is 90.90%.

❗ Current head 3141742 differs from pull request most recent head 2a2f044. Consider uploading reports for the commit 2a2f044 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1420   +/-   ##
=========================================
  Coverage     33.71%   33.72%           
- Complexity    16854    16861    +7     
=========================================
  Files          1051     1051           
  Lines        162290   162314   +24     
  Branches      34867    34867           
=========================================
+ Hits          54715    54737   +22     
  Misses        98191    98191           
- Partials       9384     9386    +2     
Impacted Files Coverage Δ
...ceforge/kolmafia/persistence/FamiliarDatabase.java 43.26% <87.50%> (+1.37%) ⬆️
src/net/sourceforge/kolmafia/Modifiers.java 65.55% <94.11%> (+0.15%) ⬆️
.../sourceforge/kolmafia/request/StandardRequest.java 81.08% <0.00%> (-2.71%) ⬇️
src/net/sourceforge/kolmafia/RequestThread.java 38.18% <0.00%> (+1.21%) ⬆️

Continue to review full report at Codecov.

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

Copy link
Member

@midgleyc midgleyc left a comment

Choose a reason for hiding this comment

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

It might be nice to handle Peppermint Rhino while you're adding fairy types: the Rhino is a 1x fairy and a 1x candy fairy (for a total of 2x candy drops).

I note this is a case currently not handled by the code -- a familiar is either a food or booze or item fairy, not a combination. It would be nice to sort out this part of the design -- for the difficulty in adding it after the fact, see how other parts of the codebase handle drunki-bears ;)

@gausie
Copy link
Contributor Author

gausie commented Jan 10, 2023

Hmm ok. This would mean I need to add Booze Fairy, Food Fairy and Candy Fairy modifiers. Does that sound like a good idea?

@midgleyc
Copy link
Member

Yeah. I can't really see another way of doing it, and I think it's better than having a Fairy type that switches based on familiar tags.

@gausie
Copy link
Contributor Author

gausie commented Jan 10, 2023

That should be it! Thanks for the feedback, this is much better. It would be great to turn Modifiers into an enum, having this numerical key is very annoying.

@gausie gausie requested a review from midgleyc January 10, 2023 15:45
src/data/familiars.txt Outdated Show resolved Hide resolved
test/net/sourceforge/kolmafia/ModifiersTest.java Outdated Show resolved Hide resolved
@midgleyc
Copy link
Member

midgleyc commented Jan 10, 2023

the enum type is in #1401, just waiting for final comments ;)

edit: oh, unless you meant Modifiers themselves, which would be much harder.

@gausie gausie requested a review from midgleyc January 10, 2023 17:02
Copy link
Member

@midgleyc midgleyc left a comment

Choose a reason for hiding this comment

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

Thanks!

@midgleyc midgleyc enabled auto-merge (squash) January 10, 2023 17:04
@midgleyc midgleyc merged commit 6766887 into main Jan 10, 2023
@midgleyc midgleyc deleted the support-booze-and-food-fairies branch January 10, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants