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

fix: limit_mode for none must be empty #1089

Merged
merged 5 commits into from
Sep 17, 2022

Conversation

midgleyc
Copy link
Member

#1084 broke ChIT, which assumes the result of limit_mode for no limit mode must be "", but it is instead "0".

@midgleyc midgleyc requested a review from a team as a code owner September 17, 2022 14:41
@gausie
Copy link
Contributor

gausie commented Sep 17, 2022

I really think this isn't right. We should keep it as 0 - the official key we get from KoL servers - and add a getASHName instance function to LimitMode that returns "" when the enum is LimitMode.NONE, and use that in limit_mode().

@codecov
Copy link

codecov bot commented Sep 17, 2022

Codecov Report

Merging #1089 (f8eddc9) into main (acdde48) will decrease coverage by 0.00%.
The diff coverage is 87.50%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1089      +/-   ##
============================================
- Coverage     29.65%   29.65%   -0.01%     
  Complexity    14350    14350              
============================================
  Files          1036     1036              
  Lines        160939   160943       +4     
  Branches      35122    35123       +1     
============================================
- Hits          47729    47728       -1     
- Misses       104649   104652       +3     
- Partials       8561     8563       +2     
Impacted Files Coverage Δ
.../sourceforge/kolmafia/request/CharPaneRequest.java 49.06% <33.33%> (+0.29%) ⬆️
...rc/net/sourceforge/kolmafia/session/LimitMode.java 70.78% <95.23%> (-0.48%) ⬇️
...sourceforge/kolmafia/textui/langserver/Script.java 81.91% <0.00%> (-3.20%) ⬇️
src/net/sourceforge/kolmafia/FamiliarData.java 53.30% <0.00%> (-0.31%) ⬇️
src/net/sourceforge/kolmafia/KoLCharacter.java 56.26% <0.00%> (-0.09%) ⬇️
...sourceforge/kolmafia/session/EquipmentManager.java 37.07% <0.00%> (+0.08%) ⬆️

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 acdde48...f8eddc9. Read the comment docs.

@midgleyc
Copy link
Member Author

"0" isn't the key, though. The key is an integer -- 0.

I'd rather change the API code to something like

    Object lmo = JSON.get("limitmode");
    if (lmo instanceof Integer && lmo.equals(0)) {
      KoLCharacter.setLimitMode(LimitMode.NONE);
    } else {
      KoLCharacter.setLimitMode(lmo.toString());
    }

and add UNKNOWN for an unknown string.

@gausie
Copy link
Contributor

gausie commented Sep 17, 2022

"0" isn't the key, though. The key is an integer -- 0.

I'd rather change the API code to something like

    Object lmo = JSON.get("limitmode");
    if (lmo instanceof Integer && lmo.equals(0)) {
      KoLCharacter.setLimitMode(LimitMode.NONE);
    } else {
      KoLCharacter.setLimitMode(lmo.toString());
    }

and add UNKNOWN for an unknown string.

This works with me

if (lmo instanceof Integer && lmo.equals(0)) {
KoLCharacter.setLimitMode(LimitMode.NONE);
} else {
KoLCharacter.setLimitMode(lmo.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably instanceof string check here and error out otherwise. And add a coverage test to this slice of the code too, why not

Copy link
Member Author

Choose a reason for hiding this comment

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

aye, will merge here and follow-up.

@midgleyc midgleyc merged commit 0bcde32 into kolmafia:main Sep 17, 2022
@midgleyc midgleyc deleted the enum-none-must-be-zero branch September 17, 2022 18:30
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