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

Cookbookbat recipe drops tracking #1252

Merged
merged 11 commits into from Nov 11, 2022
Merged

Conversation

Zauschneria
Copy link
Contributor

Adding a property to track cookbookbat recipe drops and setting it up as a drops familiar

@Zauschneria Zauschneria requested a review from a team as a code owner November 9, 2022 01:45
@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #1252 (138b399) into main (6726843) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1252      +/-   ##
============================================
+ Coverage     32.47%   32.50%   +0.03%     
  Complexity    16060    16060              
============================================
  Files          1042     1042              
  Lines        162349   162355       +6     
  Branches      35251    35254       +3     
============================================
+ Hits          52727    52781      +54     
+ Misses       100349   100290      -59     
- Partials       9273     9284      +11     
Impacted Files Coverage Δ
.../net/sourceforge/kolmafia/objectpool/ItemPool.java 14.28% <ø> (ø)
src/net/sourceforge/kolmafia/FamiliarData.java 57.68% <100.00%> (+0.34%) ⬆️
.../sourceforge/kolmafia/session/ResultProcessor.java 25.73% <100.00%> (+0.08%) ⬆️
...sourceforge/kolmafia/textui/langserver/Script.java 81.91% <0.00%> (-3.20%) ⬇️
...rceforge/kolmafia/persistence/HolidayDatabase.java 60.20% <0.00%> (-0.52%) ⬇️
src/net/sourceforge/kolmafia/KoLCharacter.java 57.92% <0.00%> (-0.09%) ⬇️
src/net/sourceforge/kolmafia/RequestThread.java 38.18% <0.00%> (+1.21%) ⬆️
...eforge/kolmafia/swingui/panel/DailyDeedsPanel.java 7.18% <0.00%> (+2.01%) ⬆️
... and 1 more

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 6726843...138b399. Read the comment docs.

@gausie
Copy link
Contributor

gausie commented Nov 9, 2022

I thought only one can drop a day? Also will this trigger from the item you get when initially using the hatchling? I assume not, but worth inspecting.

@Zauschneria
Copy link
Contributor Author

Yeah, it is set up for there to be 1 recipe drop per day, I could switch the property to a boolean, but with a counter it would make any accidental overcounting really obvious. I added the check for active familiar when updating the property which should prevent that first recipe from counting. I would be interested to see if buying a recipe from the mall with a cookbookbat active causes issues, but it seems like a pretty niche edge case.

@Veracity0
Copy link
Contributor

Actually, since you have that inside "if (adventureResults)", that should be enough.
That means the drop came as the result of adventuring - usually combat, but I think NCs count there too.
In particular, it does not include buying from the mall or, resumably, growing the familiar larva.

@gausie
Copy link
Contributor

gausie commented Nov 9, 2022

Make it a boolean pref. If you're worried about overcounting add test cases to confirm that it doesn't happen!

@Zauschneria
Copy link
Contributor Author

So the drops familiar logic only appears to work with numbers https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/swingui/panel/DailyDeedsPanel.java#L2818-L2836 so it has to be an int without some more significant refactoring, you can see that there are already other drop familiars with only one drop per day: https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/FamiliarData.java#L1092-L1098

src/data/defaults.txt Outdated Show resolved Hide resolved
@libraryaddict
Copy link
Contributor

Would this work as a solution for https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/FamiliarData.java#L1014 or would it be too messy/hacky?

    public int dropsToday() {
      if (Preferences.getDefault(this.dropTracker).equals("false")) {
        return Preferences.getBoolean(this.dropTracker) ? 1 : 0;
      }

      return Preferences.getInteger(this.dropTracker);
    }

@gausie
Copy link
Contributor

gausie commented Nov 10, 2022

Would this work as a solution for https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/FamiliarData.java#L1014 or would it be too messy/hacky?

    public int dropsToday() {
      if (Preferences.getDefault(this.dropTracker).equals("false")) {
        return Preferences.getBoolean(this.dropTracker) ? 1 : 0;
      }

      return Preferences.getInteger(this.dropTracker);
    }

I think that's exactly the right solution

@Zauschneria
Copy link
Contributor Author

I will convert all the other 1 drop familiars to booleans in another PR

@midgleyc midgleyc enabled auto-merge (squash) November 11, 2022 17:30
@midgleyc midgleyc merged commit 49b612a into kolmafia:main Nov 11, 2022
@Zauschneria Zauschneria deleted the cookbookbat branch November 16, 2022 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants