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

Improvements to the CMC command #959

Merged
merged 7 commits into from
Aug 10, 2022

Conversation

Aenimus
Copy link
Contributor

@Aenimus Aenimus commented Aug 8, 2022

Newly visited locations are appended to the right; rectified the pill prediction logic accordingly.

Accounted for unknown locations.
Added line breaks to predictions.
Added more tests.
Removed unused constant.

… prediction logic accordingly.

Accounted for unknown locations.
Added line breaks to predictions.
Added more tests.
Removed unused constant.
@Aenimus Aenimus requested a review from a team as a code owner August 8, 2022 23:22
@Aenimus
Copy link
Contributor Author

Aenimus commented Aug 8, 2022

@jaadams5 I thought new state was appended to the left, not the right. I've changed the logic accordingly (it actually made it simpler). Sorry.

@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #959 (1d9062c) into main (90d53ae) will decrease coverage by 0.00%.
The diff coverage is 93.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #959      +/-   ##
============================================
- Coverage     27.11%   27.11%   -0.01%     
+ Complexity    12697    12696       -1     
============================================
  Files          1029     1029              
  Lines        160332   160336       +4     
  Branches      35274    35275       +1     
============================================
- Hits          43480    43478       -2     
- Misses       108918   108919       +1     
- Partials       7934     7939       +5     
Impacted Files Coverage Δ
...fia/textui/command/ColdMedicineCabinetCommand.java 96.93% <93.33%> (-1.50%) ⬇️
...et/sourceforge/kolmafia/objectpool/Concoction.java 59.48% <0.00%> (-0.55%) ⬇️
src/net/sourceforge/kolmafia/KoLCharacter.java 50.36% <0.00%> (-0.05%) ⬇️
...rceforge/kolmafia/persistence/HolidayDatabase.java 34.81% <0.00%> (+0.23%) ⬆️

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 90d53ae...1d9062c. Read the comment docs.

@gausie
Copy link
Contributor

gausie commented Aug 9, 2022

I think it would be good to put this functionality behind a param. Like cmc needed or something

Added separate command for pills.
@gausie gausie changed the title Newly visited locations are appended to the right; rectified the pill… Improvements to the CMC command Aug 9, 2022
@gausie
Copy link
Contributor

gausie commented Aug 9, 2022

@jaadams5 please don't merge the branch while people are working on it

Changed pills command to plan
@gausie
Copy link
Contributor

gausie commented Aug 9, 2022

Looks fantastic! All we need now is a little bit of inline documentation on what the nature of some of the more complex functions are actually doing - this is quite complex and it would be good for someone coming back for maintenance in the future.

Aenimus and others added 2 commits August 9, 2022 16:33
Moved output out of populateNaiveTurnsRequiredForPillMap
Handled case where there is already a majority
@Aenimus
Copy link
Contributor Author

Aenimus commented Aug 9, 2022

@jaadams5 This is now complete and ready for review/merge. :)

@gausie gausie self-requested a review August 10, 2022 10:47
@gausie gausie enabled auto-merge (squash) August 10, 2022 10:48
@gausie gausie merged commit c45718b into kolmafia:main Aug 10, 2022
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