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

fix(types): SentinelWeapons typing #485

Closed
wants to merge 2 commits into from

Conversation

mdpung
Copy link

@mdpung mdpung commented Oct 17, 2023

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

  • Does this contain a new dependency? [No]
  • Does this introduce opinionated data formatting or manual data entry? [No]
  • Does this pr include updated data files in a separate commit that can be reverted for a clean code-only pr? [Yes]
  • Have I run the linter? [Yes]
  • Is is a bug fix, feature request, or enhancement? [Bug Fix]

@mdpung mdpung requested a review from a team as a code owner October 17, 2023 06:06
@mdpung mdpung requested a review from EricSihaoLin October 17, 2023 06:06
index.d.ts Show resolved Hide resolved
@mdpung
Copy link
Author

mdpung commented Oct 17, 2023

Also a fix for inconsistency mentioned in #482

Copy link
Member

@TobiTenno TobiTenno left a comment

Choose a reason for hiding this comment

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

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

@mdpung
Copy link
Author

mdpung commented Oct 17, 2023

  • 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.

Since SentinelWepeapons is a category and productCategory, the type SentinelWeapon will reflect that.
@TobiTenno
Copy link
Member

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

@mdpung
Copy link
Author

mdpung commented Oct 17, 2023

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

@mdpung
Copy link
Author

mdpung commented Oct 17, 2023

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 TobiTenno closed this Oct 18, 2023
@TobiTenno TobiTenno reopened this Oct 18, 2023
@TobiTenno
Copy link
Member

TobiTenno commented Oct 18, 2023

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
Copy link
Member

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

@TobiTenno TobiTenno closed this Oct 18, 2023
@mdpung
Copy link
Author

mdpung commented Oct 18, 2023

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
Copy link
Member

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
Copy link
Member

TobiTenno commented Oct 18, 2023

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

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