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

Implement #26: set_aside multiple items in a list #29

Closed
wants to merge 0 commits into from

Conversation

haggys22
Copy link

  • state.aside is now a list
  • set_aside pushes the current item as the head
  • with_aside fails if the list is empty; uses head otherwise
  • awaken and recombine consume the head of the list
  • beastcraft_split pushes the split item as the head of the list
  • swap replaces the current item with the head of the list
    • if there is no current item: removes the head of the list, if not empty
    • if the list is empty: sets the current item to None

I started learning OCaml just after opening the issue, so please handle this PR with care. It seems to work just fine but feel free to refactor, give advice on how to do it better, etc.

Test cases:

swap, set_aside, recombine:

# current item = None; set aside = []
swap # None; []

buy "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand13" # A; []
swap # None; [A]
swap # A; []
set_aside # None; [A]

buy "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand16" # B; [A]

recombine # AB; []
set_aside # None; [AB]

buy "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand13" # C; [AB]
set_aside # None; [C, AB]

buy "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand15" # D; [C, AB]

recombine # CD; [AB]
swap # AB; [CD]

recombine # ABCD; []

recombine # Fails

beastcraft_split, awaken:

# None; []
buy "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand13" # A; []
beastcraft_split # A1; [A2]

buy warlord "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand13" # B; [A2]
swap # A2; [B]

buy crusader "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand16" # C; [B]

awaken # BC; []

buy crusader "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand16" # D; []

awaken # Fails

@doomeer
Copy link
Owner

doomeer commented Jun 19, 2022

Thanks for your contribution! And thanks for the great description of the feature and test cases.

I actually had implemented exactly the same feature a few days ago and forgot to commit and push. Well, I didn't write the doc, which is why I didn't push. Your PR motivated to finish my work and push it ;)

Your code looks perfectly fine. I compared it with mine and it is quite close, but there are some interesting differences if you want to have a look. I'm not saying my version is better to be honest. I just share a bit more code. One key difference though is that in my version, I fail in more cases. For instance, trying to swap with no item fails. I tried your first test case and it showed this difference. There are probably other cases where I don't handle errors in the same way.

I'll leave this PR open in case you think that your approach to handle errors is better (i.e. don't fail if stack is empty) so we can discuss it.

@haggys22
Copy link
Author

First of all thanks for quickly implementing (or pushing) this and my other request.

The reason for not failing when there is no current item or no set aside item was mainly because it didn't as of commit 3088650:

echo "start"
show
swap
echo "swap"
show

buy "Metadata/Items/Weapons/OneHandWeapons/Wands/Wand13"
echo "buy"
show
swap
echo "swap"
show
swap
echo "swapped again"
show

resulted in

start
(no current item)
swap
(no current item)
buy
--------
Imbued Wand (Rare)
--------
(prefix) [T1] 0.3% of Physical Attack Damage Leeched as Mana
(prefix) [T9] Adds 7 to 14 Cold Damage
(prefix) [T12] +30 to maximum Mana
(suffix) [T5] +28% to Cold Resistance
(suffix) [T2] 15% chance to Shock
--------
Paid up to now: 0.00ex (0c)
swap
(no current item)
swapped again
--------
Imbued Wand (Rare)
--------
(prefix) [T1] 0.4% of Physical Attack Damage Leeched as Mana
(prefix) [T9] Adds 7 to 14 Cold Damage
(prefix) [T12] +38 to maximum Mana
(suffix) [T5] +25% to Cold Resistance
(suffix) [T2] 15% chance to Shock
--------
Paid up to now: 0.00ex (0c)
--------
Imbued Wand (Rare)
--------
(prefix) [T1] 0.22% of Physical Attack Damage Leeched as Mana
(prefix) [T9] Adds 7 to 15 Cold Damage
(prefix) [T12] +32 to maximum Mana
(suffix) [T5] +27% to Cold Resistance
(suffix) [T2] 15% chance to Shock
--------
Cost:
Total: 0.00ex (0c)

IMO warning instead of failing might be the best solution for backwards compatibility with old recipes that, for whatever reason, relied on this behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants