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

Sort merchants to the top of crafting steps #32

Merged

Conversation

darthmaim
Copy link
Contributor

This always sorts merchants that only require wallet currencies to the top of the crafting steps.

This does not really implement what gw2efficiency/issues#1848 was asking for, but is an easy first step.

The existing test for mystic clovers covers the positive case for this and allows us to remove the special handling for Obsidian Shards and Philosopher Stones.

We should add a test that makes sure that vendors that require items are not sorted to the top still. How do you best generate the tree for the test?

Also should verify that the crafting calculator never suggests crafting currencies!

Copy link
Member

@queicherius queicherius left a comment

Choose a reason for hiding this comment

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

Thanks for this! It already looks good and I'd be happy to merge as a first step as-is, or let me know if you wanna implement what you mentioned in the comment.

[...] allows us to remove the special handling for Obsidian Shards and Philosopher Stones.

💯

We should add a test that makes sure that vendors that require items are not sorted to the top still. How do you best generate the tree for the test?

👍 I usually call the https://api.gw2efficiency.com/recipes/101882 endpoint (item id, not recipe id) and copy/paste it in & strip out what's not needed, or copy/paste existing trees and adjust them.

@darthmaim
Copy link
Contributor Author

darthmaim commented Nov 23, 2024

I will add a test tomorrow and then this can be merged as a first step.

Or if you want you can just merge this now and I will add the test in a follow-up PR.

@darthmaim darthmaim force-pushed the feature/sort-merchants-to-top branch from af27efe to 94abe01 Compare November 23, 2024 14:23
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (d180b81) to head (94abe01).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #32   +/-   ##
=======================================
  Coverage   98.57%   98.57%           
=======================================
  Files          15       15           
  Lines         210      211    +1     
  Branches       68       70    +2     
=======================================
+ Hits          207      208    +1     
  Misses          3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@darthmaim darthmaim marked this pull request as ready for review November 23, 2024 14:25
@darthmaim
Copy link
Contributor Author

darthmaim commented Nov 23, 2024

I've added a test using this simple recipe: https://gw2efficiency.com/crafting/calculator/a~0!b~1!c~0!d~1-91571!e~1. As you can see in the snapshot the merchant is the last step and not at the top :)

I've ended up copying the tree from the console output of the crafting calculator and removing unnecessary fields, because https://api.gw2efficiency.com/recipes/91571 was missing too many :(

Should be good now 👍

Copy link
Member

@queicherius queicherius left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! ❤️

@queicherius queicherius merged commit 25d414a into gw2efficiency:master Nov 23, 2024
4 checks passed
@darthmaim darthmaim deleted the feature/sort-merchants-to-top branch November 23, 2024 20:41
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