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

Add loot& and transfer& commands #3287

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

dm1tz
Copy link
Contributor

@dm1tz dm1tz commented Sep 9, 2024

Checklist

  • I read and understood the Contributing Guidelines.
  • This is not a duplicate of an existing merge request.
  • I believe this falls into the scope of the project and should be part of the built-in functionality.
  • My code follows the code style of this project.
  • I have added tests to cover my changes, wherever they are necessary.
  • All new and existing tests pass.

Changes

New functionality

Adds two derivatives of loot^ and transfer^ commands: loot& and transfer&, allowing one to transfer Steam items that match user-provided set of EAssetRarity.

Additional info

This implementation expands the current EAssetRarity enum with rarities inherent in official Valve games, namely: CS2, DOTA2 and TF2, as well as some non-Valve games that I'm somewhat familiar with (eg. PAYDAY2). Covering every possible item rarity in non-Valve games that support Steam Inventory Service is pretty unrealistic since, to my knowledge, there's no strict naming conventions.

Some clarifications:

  • Seasonal (DOTA2) and Default rarities are intentionally omitted, they are not tradable.
  • tag.category.ToUpperInvariant() is used because non-Valve games tend to have lowercase rarity category. Conversely, the same category in Valve games is Rarity.

Also, I'm not sure about Arcana rarity. It is DOTA2 specific, maybe it would be better to combine it with Unusual? Would greatly appreciate any kind of feedback.

Copy link
Member

@JustArchi JustArchi left a comment

Choose a reason for hiding this comment

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

Other than mentioned concerns it looks good 🙂

ArchiSteamFarm/Steam/Data/InventoryDescription.cs Outdated Show resolved Hide resolved
ArchiSteamFarm/Steam/Interaction/Commands.cs Outdated Show resolved Hide resolved
ArchiSteamFarm/Steam/Interaction/Commands.cs Outdated Show resolved Hide resolved
ArchiSteamFarm/Steam/Interaction/Commands.cs Outdated Show resolved Hide resolved
ArchiSteamFarm/Steam/Interaction/Commands.cs Outdated Show resolved Hide resolved
@JustArchi JustArchi added 🔧 Fixes required Issues marked with this label require further follow-up fixes before they can be considered. 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. labels Sep 9, 2024
@dm1tz
Copy link
Contributor Author

dm1tz commented Sep 9, 2024

@JustArchi, thanks a lot for the suggestions! Hope I got them right.

@JustArchi JustArchi added 🏁 Finished Issues marked with this label were finished already and no further work is required on them. and removed 🔧 Fixes required Issues marked with this label require further follow-up fixes before they can be considered. labels Sep 9, 2024
@dm1tz
Copy link
Contributor Author

dm1tz commented Sep 10, 2024

Will remove the trailing // in the next push, don't want to trigger CI twice in case there are new suggestions.

Copy link
Contributor

@Abrynos Abrynos left a comment

Choose a reason for hiding this comment

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

LGTM

@Rudokhvist
Copy link
Contributor

I looked at the code and it looks fine (no wonder after Abrynos' and Archi's reviews), but to be honest I don't understand reasoning behind those commands and chosen way in which they work. First of all, ASF is a card farming app, not general account management tool, shouldn't it be in a plugin instead? And, secondly, why we try to convert rarities to enum, instead of just filtering by internal name (how user is supposed to know this name is another question, but understanding the match between enum and shown rarity is not straighforward anyway, if I understand it right). If we pick by internal name, wouldn't it be more generic and applicable to any game inventory, not just to Valve's games?

@dm1tz
Copy link
Contributor Author

dm1tz commented Sep 11, 2024

First of all, ASF is a card farming app, not general account management tool, shouldn't it be in a plugin instead?

As I stated earlier, added commands are derivatives of loot^ and transfer^, if they were shipped as plugins, I would happily do as you proposed. Besides:

C# application with primary purpose of farming Steam cards from multiple accounts simultaneously.


how user is supposed to know this name is another question

Some variation of this where we can state the list of all available rarities, aliases and internal names.

If we pick by internal name, wouldn't it be more generic and applicable to any game inventory, not just to Valve's games?

I don't think it would be a good idea to have something like Rarity_Common_Weapon as a valid user input. Additionally, matching directly from internal name would negate the usage of numeric input.

@Abrynos
Copy link
Contributor

Abrynos commented Sep 12, 2024

if they were shipped as plugins, I would happily do as you proposed

Still possible to extract them into official plugin shipped with ASF 😉

@JustArchi
Copy link
Member

Thanks for your contribution 🏆

@dm1tz dm1tz deleted the transfer-by-asset-rarity branch September 13, 2024 07:30
JustArchi added a commit that referenced this pull request Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Enhancement Issues marked with this label indicate further enhancements to the program, such as new features. 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. 🏁 Finished Issues marked with this label were finished already and no further work is required on them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants