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

@pkmn/sets: functionality of Sets functions don't match types #37

Open
Gudine opened this issue Mar 2, 2025 · 1 comment
Open

@pkmn/sets: functionality of Sets functions don't match types #37

Gudine opened this issue Mar 2, 2025 · 1 comment
Labels
bug Something isn't working

Comments

@Gudine
Copy link

Gudine commented Mar 2, 2025

Describe the bug:

The return value of some of the functions in @pkmn/sets don't match the types assigned to them. Specifically:

  • Sets.unpackSet's return potentially lacks two properties from PokemonSet: gender and level;
  • Sets.importSet returns undefined when given a string containing only spaces;
  • Sets.canonicalize throws when given a PokemonSet partial that doesn't have species or moves — the latter in particular can happen when giving it the output of Sets.importSet.

Example:

const set1 = Sets.unpackSet("Espeon|||-||||||||");

console.log(set1.gender, set1.level);
// undefined, undefined

const set2 = Sets.importSet("");

console.log(set2);
// undefined

Sets.canonicalize(Sets.importSet("Tangrowth"), Dex);
// Uncaught TypeError: s.moves is not iterable

Expected behavior:

The functions' parameter and return types should match their implementations.

Additional context:

I'd be willing to open a PR to fix the issues, but I'd have to know whether the types or the implementations should be changed.

@Gudine Gudine added the bug Something isn't working label Mar 2, 2025
@scheibo
Copy link
Contributor

scheibo commented Mar 3, 2025

Thanks for taking an interest in this. The challenge here is the upstream Pokemon Showdown's PokemonSet type doesnt really reflect the requirements.

  • species and moves are always required
  • attempting to import anything which does not contain these (eg an empty string) should throw as illegal
  • unpacking the set unfortunately cant fill in the gender/level if unspecified, so these should perhaps be marked as optional (they get filled in during team validation)

The reason these types are pretty loose sadly is that so many of the fields are actually optional that it makes it annoying to use the types because you almost always are going to ! them (why I believe is why upstream these types work the same way they do here - they dont actually type the thing theyre purporting to)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants