-
Notifications
You must be signed in to change notification settings - Fork 2
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
Check for hyper training legality #22
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Sorry about the delay. I have a few recommendations.
src/checks/hyper-training-6iv.js
Outdated
pkmn.hyperTrainedDef || pkmn.hyperTrainedSpAtk || | ||
pkmn.hyperTrainedSpDef || pkmn.hyperTrainedSpe); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check seems redundant with hyper-training-31. Anything that it marks illegal will also be marked illegal by that other check.
src/checks/hyper-training.js
Outdated
}, | ||
field: 'level', | ||
test (pkmn) { | ||
return !(pkmn.hyperTrainedHP || pkmn.hyperTrainedAtk || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change hyperTrainedHP
to hyperTrainedHp
? We recently added that as an alias for consistency with the other stuff. (You might need to run npm install pkparse@latest
.)
src/checks/hyper-training-31.js
Outdated
field: 'iv', | ||
test (pkmn) { | ||
const maxIV = { | ||
HP: pkmn.ivHp === 31, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're using HP
as the key here, but Hp
when you reference it below.
src/checks/hyper-training-31.js
Outdated
SpDef: pkmn.ivSpDef === 31, | ||
Spe: pkmn.ivSpe === 31 | ||
}; | ||
return !(maxIV.Hp ? pkmn.hyperTrainedHp : maxIV.Hp || maxIV.Atk ? pkmn.hyperTrainedAtk : maxIV.Atk || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these ternary expressions are mostly redundant -- if maxIV.Hp
is false, then there's no reason to negate it again. This check could be simplified to
return !(
pkmn.ivHp === 31 && pkmn.hyperTrainedHp ||
pkmn.ivAtk === 31 && pkmn.hyperTrainedAtk ||
pkmn.ivDef === 31 && pkmn.hyperTrainedDef ||
pkmn.ivSpAtk === 31 && pkmn.hyperTrainedSpAtk ||
pkmn.ivSpDef === 31 && pkmn.hyperTrainedSpDef ||
pkmn.ivSpe === 31 && pkmn.hyperTrainedSpe
);
src/checks/hyper-training-31.js
Outdated
module.exports = { | ||
description: 'A maxed IV (31) cannot be hypertrained.', | ||
filter (pkmn) { | ||
return pkmn.ivHp === 31 || pkmn.ivAtk === 31 || pkmn.ivDef === 31 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this filter is kind of unnecessary here given that it needs to also be run in the test function anyway. It might be better to remove it.
33db1c9
to
2436ba3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic makes sense, but I'm not a fan of how complicated it is to read through it.
module.exports = { | ||
description: 'A Pokémon from SuMo can only be hyper trained if level 100.', | ||
filter (pkmn) { | ||
return pkmn.level !== 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this filter and test. It's quite complicated to read. We don't want to play coding golf here.
Filter on the game, imo.
Then do something like
if (pokemon.level === 100) {
return true
}
if (pokemon.hyperTrained || .. ) {
return false
)
That makes it really clear what it's doing. As I read through this, it's really complicated to figure out what the logic is actually doing, it kinda feels unnecessary to make it so concise, and also we don't want to check non-gen 7 pokemon.
filter (pkmn) { | ||
return pkmn.level !== 100; | ||
}, | ||
field: 'level', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really level, is it? It's hypertrained that's the issue.
field: 'iv', | ||
test (pkmn) { | ||
return !( | ||
pkmn.ivHp === 31 && pkmn.hyperTrainedHp || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rework the logic on this one as well, please. It's also really confusing to do.
Fitler on gen 7, and make it similar to how I did the other one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to clarify - should it be done like the other test where it's split in their separate cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
Checks for...