WFCD / warframe-items

📘 Get all Warframe items directly from Warframe's API. No more messy wikia scraping.
MIT License
277 stars 53 forks source link

fix(types): SentinelWeapons typing #485

Closed mdpung closed 1 year ago

mdpung commented 1 year ago

What did you fix?

SentinelWeapons was incorrectly typed as it was not typed as a Category and should be as it behaves similarly to "Primary" and "Secondary" categories.


Reproduction steps

import Items, { Category } from "warframe-items";

// Originally, this instantiation will not work because "SentinelWeapons" is not typed as a Category but should be.
const weaponTypes: Category[] = ["Primary", "Secondary", "Melee", "Arch-Gun", "Arch-Melee", "SentinelWeapons"];
const items = new Items({ category:  });

Evidence/screenshot/link to line

image

Considerations

mdpung commented 1 year ago

Also a fix for inconsistency mentioned in #482

mdpung commented 1 year ago
  • SentinelWeapons is a ProductCategory

  • SentinelWeapons is a valid entry to the categories array, but is a ProductCategory

I see that the productCategory is defined by DE, so I'll leave it as a productCategory. But since it's also a category, it's not properly typed as a category. I will make the update and see if that's more acceptable.

TobiTenno commented 1 year ago

that's fine if you do that on your branch, but i'm not going to merge it.

mdpung commented 1 year ago

Can I ask why? There's literally bugs in the code.

mdpung commented 1 year ago

All the change is, is changing the SentinelWeapon type category from "Primaries" to "SentinelWeapon" (you said it was wrong yourself) and making SentinelWeapon a proper type for category.

TobiTenno commented 1 year ago

Because I said it shouldn't be changed. Yes, I'm annoying, sorry. My point is you're crossing streams of a ProductCategory into Category, and I'm telling you I don't want it.

It's also not "literally bugs in the code", considering you're playing with types, you're not even fixing build code

TobiTenno commented 1 year ago

This change will break more then it fixes so at the time being this isn't a critical fix

mdpung commented 1 year ago

SentinelWeapons was made a Category and ProductCategory by design so I would say those streams were already crossed lol. I'll just do my own data repository, this project seems to be causing me more headaches than what it's worth.

TobiTenno commented 1 year ago

it's literally not a category. it's an option for the category argument because there's a file created for it. that's not the same as a member of the Category type

TobiTenno commented 1 year ago

your changes weren't even passing the type checks, so i wouldn't have merged it even if i had started agreeing with you that this should get fix immediately image