Enable test_checkMissingTokenData #14316
Conversation
…hecks to establish a baseline for regressions
| // // how-to fix: | ||
| // // - public token must be downloadable, so tok-data must contain miss set | ||
| // // (also don't forget to add new set to scryfall download) | ||
| // errorsList.add("Error: can't find data in tokens-database.txt for token: " + tokenClass.getName() + " -> " + token.getName()); |
There was a problem hiding this comment.
Thats wrong fix style.
If you need to disable critical checks then must temporary move it to warningsList and add todo about fix and return to errorsList after project fix.
Commented code is bad thing and can be removed as dirty/outdated code by someone later, but it’s not outdated here, it’s just require a project fixes before enable.
Another approach is feature flag (boolean const) to enable/disable that check
There was a problem hiding this comment.
Yeah, I don't think this code is outdated, but we definitely don't want it blocking right now either.
I am willing to take the time to fix the underlying things that block enabling these checks too, but in the mean time think the other parts of this check are worth having. 💪
Happy to refactor this as advised, either through demoting to warnings for now, or hiding behind flags, until I get around to fixing them, having the code stable, and the checks pass when executed.
There was a problem hiding this comment.
Moved behind a flag with a note that the first thing anyone (probably me 😁) should do is address private tokens being declared instead of CreatureToken being used.
When correcting to using CreatureToken, fixes can be made for offending naming styles etc too which will greatly reduce the number of classes to address that are flagged for multiple check violations.
2 participants
I'm eager to actually have this test run and deliver value given various other bits of tinkering I've done, and plan to do around tokens.
I think there's some value to this test as is, especially around stating what desired state is even if the codebase isn't there today.
So;
@IgnoredRunning this locally, it passes on the tip of
master, albeit it does produce a number ofinfoandwarningmessages that are visible in the surefire reports. I think this is fine, and do not think it's worth addressing them all quite yet as some of these are tied up in the more fundamental and far reaching aim to move things that should be usingCreatureTokento doing that.