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 methods to generate notable move categories #14

Merged
merged 22 commits into from
Sep 3, 2024

Conversation

shrianshChari
Copy link
Collaborator

@shrianshChari shrianshChari commented Aug 28, 2024

Partially fixes #6

@scheibo
Copy link
Contributor

scheibo commented Aug 28, 2024

Thanks for attempting this, but theres quite a lot that needs to be fixed here.

  1. You haven't added any tests, if you did you would see your sets disagree with those that already exist
  2. You aren't actually using any of the tables youre computing
  3. You cannot use @pkmn/dex outside of tests in this codebase (and why would you want to?)
  4. Please do not open PRs before running the linter and test suite locally
  5. This PR does not close Update the classifier to support modern Generations #6, there are many more tables that need to be replaced. You also seem to be continuing to work on the PR, please open the PR as a draft if it is actually not finished.

I've pushed some changes that should start you on a better footing. Note that Antar's sets might be incorrect, if so just change the test asserts to be like to be like:

 expect(classifier.computeGreaterSetupMoves(GEN)).toEqual([...classifier.GREATER_SETUP_MOVES, ...additionalMovesAnatarShoildHaveAdded]);

etc

@scheibo scheibo marked this pull request as draft August 29, 2024 02:56
@shrianshChari shrianshChari marked this pull request as ready for review August 31, 2024 01:36
@shrianshChari
Copy link
Collaborator Author

The other lists don't contain fields that I can use to filter the correct moves; I have hypothesized that I can try and filter based on the shortDesc of moves, items, and abilities but will leave that for other PRs.

@scheibo
Copy link
Contributor

scheibo commented Sep 3, 2024

Great work

I have hypothesized that I can try and filter based on the shortDesc of moves, items, and abilities but will leave that for other PRs.

A clever idea, but I dont think we should rely on the descriptions as they are completely subjective

@scheibo scheibo merged commit f81ee37 into pkmn:main Sep 3, 2024
1 check passed
@scheibo scheibo deleted the classifier-moderngens branch September 3, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Update the classifier to support modern Generations
2 participants