Conversation
Copied and pasted my existing conflict locating function into the npm run card-diff commands.
…b.com/SWU-Karabast/forceteki into 636-fix-card-data-and-find-mismatches
scripts/carddiff.js
Outdated
| var cards = []; | ||
| if (totalPages > 0) { | ||
| try { | ||
| cards = (await Promise.all([...Array(totalPages).keys()].map((i) => axios.get(apiUrl + '?pagination[page]=' + (i + 1)).then((res) => res.data.data)))).flat(); |
There was a problem hiding this comment.
Definitely don't need to do the full integration with fetchdata in this PR, but if possible I'd prefer to avoid checking in duplicate code for downloading the card data. Could we go ahead and factor out getCardData() and the related functions to a common script that could be called from here and also from fetchdata?
One change we could make to make that easy is instead of explicitly calling filterValues() from getCardData(), we have an optional parameter to pass a handler to call on each card. Then fetchcard can send filterValues() as that handler.
scripts/carddiff.js
Outdated
| }; | ||
| for (let i = 0; i < cards.length; i++) { | ||
| const card = cards[i].attributes; | ||
| const cardIdentifier = card.title.toUpperCase() + (card.subtitle !== null && card.subtitle !== '' ? ' - ' + card.subtitle.toUpperCase() : ''); |
There was a problem hiding this comment.
We should just reuse the existing internalName format which is functionally the same as this, but is a format commonly used everywhere in the repo. Could factor the logic for generating it out of filterValues() into a method in the common script.
scripts/carddiff.js
Outdated
| upgradePower: card.upgradePower, | ||
| cardUid: card.cardUid | ||
| }; | ||
| for (let j = 0; j < card.arenas.data.length; j++) { |
There was a problem hiding this comment.
All this duplicated code for each property will be a headache to maintain. There's a few ways we could cut down on the duplicated code - have a helper method that we pass each property to, put all of the properties in a list and make a for loop, etc. Same note for all the areas below where there are large blocks of repeated code.
scripts/carddiff.js
Outdated
| countMatching++; | ||
| } | ||
|
|
||
| if (countMatching === 13) { |
There was a problem hiding this comment.
Instead of explicitly checking the number 13 (which would need to be updated if we add / remove properties), could we just use a bool like conflictFound that we set to true if there is a mismatch on any property?
scripts/carddiff.js
Outdated
| var verified = {}; | ||
| var verifiedData = {}; | ||
| var verifiedResults = { | ||
| matching: {}, |
There was a problem hiding this comment.
For the purposes of this tool I don't think there's value in saving the matching card data, we're only interested in the conflict data
| } | ||
|
|
||
| const conflicts = getConflicts(cards, false); | ||
| console.dir(conflicts.conflicting, { depth: null, color: true }); |
There was a problem hiding this comment.
Could we add an optional flag to write this to a file?
scripts/carddiff.js
Outdated
| if (verifiedResults.conflictingCount > 0) { | ||
| console.log('Found some conflicts. Run `npm run card-diff` to see the specifics.'); | ||
| } | ||
| return (verifiedResults.conflictingCount > 0); |
There was a problem hiding this comment.
I'd prefer not to use this approach because it means that the return type of the method will be completely different depending on the parameter, which makes it hard to code around correctly. We could potentially still have the flag parameter and still just return verifiedResults and the caller can ignore it if they don't plan to use it, or the caller can manage it themselves based on the value of verifiedResults.
scripts/carddiff.js
Outdated
| const { Agent } = require('https'); | ||
|
|
||
| function getConflicts(cards, flag = true) { | ||
| if (cards.length > 0) { |
There was a problem hiding this comment.
Imo it would be better to do it this way:
if (cards.length === 0) {
throw new Error("No card data found");
}
// ... function continues hereThat will (a) give us better safety if the card data is empty for some reason, and (b) make this more readable by not wrapping the whole function body in an if block
This adds in a lot of extra changes, and addresses the following comments: - First pass at separating the api calls into one function "retrieveCards" that takes in a "filtered" parameter currently used to indicate whether or not it duplicates would be removed. Required functions from fetchdata have been brought in as well. - Created function "getInternalName" which takes in the title and subtitle and outputs the key to use across all functions. Can be used anywhere and adjusting the outputs will update everywhere. - Did not update the property mess. Not 100% sure, but will investigate how it is done elsewhere to try and duplicate it. - As advised, removed all of the matching functionality and other bloat from the compare function. Now just returns data that doesn't match. - Added functionality to accept a string file location to write outputs to. - Asked for clarification on the return type of the comparisons. - Better error checking to avoid giant if blocks.
|
@AMMayberry1 Added several updates and pushed so you could take a look. This adds in a lot of extra changes, and addresses the following comments:
|
2 participants
Run "npm run card-diff" to view the output. This should take in an array of cards and build the comparisons between all variants of the card.
Still needed: Check the card variants in "npm run get-cards" BEFORE filtering to one item per card (needs to be run as all variants returned from the API to compare all of the data between all variants)