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

When a consequence sets a property with a value that is numeric, use setInteger #1478

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

Veracity0
Copy link
Contributor

@Veracity0 Veracity0 commented Jan 29, 2023

  1. Convention Hall Lobby is DiffLevel mid
  2. consequences.txt sets properties based on text in quest logs, item descriptions, effect descriptions, skill descriptions, and so on. Many of them are numeric, and the Pattern will include something like (\d+). A handful of them have ([d,]+).

In particular:

QUEST_LOG	elfGratitude	You earned ([\d,]+) Elf Gratitude during Crimbo 2022.	elfGratitude=$1
QUEST_LOG	pingpongSkill	You have achieved a skill level of ([\d,]+) at playing ping-pong	pingpongSkill=$1
QUEST_LOG	royalty	You have accumulated ([\d,]+) Royalty	royalty=[$1]
DESC_ITEM	bone abacus	defeated ([\d,]+) opponent	boneAbacusVictories=$1
DESC_ITEM	familiar scrapbook	Scraps Collected: <b>([\d,]+)</b>	scrapbookCharges=[stripcommas($1)]

I notice that the last two are in [], which means they are evaluated as a ModifierExpression.
One of them uses "stripcommas" and the other does not.
The first three are simply stored as parsed.

That confused several programmers, one of whom declared that "it is in the contract of Preferences that a numeric property's value has no commas". Bold statement, and not true, to my knowledge, but, whatever. We can do it that way to make JS coders happier.

This PR will look at the (evaluated) value and if it isNumeric (defined as "an integer, possibly with commas"), parse it as an integer (which strips commas), and store as an integer. If you later retrieve it as a String, there will be no commas.

@Veracity0 Veracity0 requested a review from a team as a code owner January 29, 2023 19:38
@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Merging #1478 (8e7ccb7) into main (3f5ac89) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1478   +/-   ##
=========================================
  Coverage     34.57%   34.57%           
  Complexity    17295    17295           
=========================================
  Files          1062     1062           
  Lines        163134   163136    +2     
  Branches      34918    34919    +1     
=========================================
+ Hits          56404    56406    +2     
  Misses        97254    97254           
  Partials       9476     9476           
Impacted Files Coverage Δ
...urceforge/kolmafia/session/ConsequenceManager.java 69.66% <100.00%> (+0.34%) ⬆️

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 3f5ac89...8e7ccb7. Read the comment docs.

@Veracity0
Copy link
Contributor Author

Note that I am willing to consider forcing individual consequences to specifically require stripping out commas.

@Veracity0
Copy link
Contributor Author

Not that I don't trust frono, but I was hoping for another review to confirm this is a reasonable solution?

Whatever. And now to bed, and I will merge this when I get up unless there is discord among my reviewers.

@midgleyc
Copy link
Member

midgleyc commented Jan 31, 2023

Note that I am willing to consider forcing individual consequences to specifically require stripping out commas.

I think that's a good idea. They are numbers, and straight numbers is nicer than numbers with commas.

I think this solution is good too.

@Veracity0 Veracity0 merged commit 47311ed into main Jan 31, 2023
@Veracity0 Veracity0 deleted the integer-consequences branch January 31, 2023 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants