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

Diff page still compares things that shouldn't be compared #18

Closed
atomicptr opened this issue May 24, 2018 · 10 comments
Closed

Diff page still compares things that shouldn't be compared #18

atomicptr opened this issue May 24, 2018 · 10 comments

Comments

@atomicptr
Copy link
Member

atomicptr commented May 24, 2018

Just seen this:

image

that shouldn't happen, have to figure out how to change that ho :>. Obviously the node didn't get renamed and all drops changed but instead it compared two completely different nodes with each other.

@atomicptr atomicptr added the bug label May 24, 2018
@TobiTenno
Copy link
Member

It's probably because they're never really sorted in a particular order....

@atomicptr
Copy link
Member Author

HM, I was more thinking in the lines of giving everything an _id, but sorting might actually be a good idea 🤔

@Senexis
Copy link
Contributor

Senexis commented May 24, 2018

The referenced PR adds an "_id" field and fixes the issue, and it would fix the issue if merged, though instead of using the implementation I suggested in the PR might not be perfect as the conversation in the PR shows. 😄

A live preview of this: https://senexis.github.io/warframe-drop-data/diff.html, though due to the newly added "_id" field it all shows as new items.

@TobiTenno
Copy link
Member

right, @atomicptr doing both.... give an id, sort by ID

@Senexis
Copy link
Contributor

Senexis commented May 24, 2018

There's no need to sort the array as long as it has IDs. Sorting it doesn't add anything because all the transients already have a unique ID at that point and is good enough.

@TobiTenno
Copy link
Member

right, but if the diff is comparing plain text and not id to id, then that would still be needed...

@Senexis
Copy link
Contributor

Senexis commented May 24, 2018

It's not. It seemingly comparing to text (it doesn't; it's the order of the array) is only being done in the case when the ID is missing, so adding an ID to the transients is really good enough.

Edit: Turns out the entire thing is being sorted already anyway.

    function sortModName(a, b) {
        if (a.modName)   { return a.modName.localeCompare(b.modName) }
        else if (a._id)  { return a._id.localeCompare(b._id) }
    }

    function sortEnemyName(a, b) {
        if (a.enemyName) { return a.enemyName.localeCompare(b.enemyName) }
        else if (a._id)  { return a._id.localeCompare(b._id) }
    }

    function sortItemName(a, b) {
        if (a.itemName)  { return a.itemName.localeCompare(b.itemName) }
        else if (a._id)  { return a._id.localeCompare(b._id) }
    }

@atomicptr
Copy link
Member Author

@Senexis your diff page actually shows the one thing that bothers me about this, instead of seeing hey shit changed for this thing you just see "THIS GOT REMOVED", "THIS GOT ADDED". :D

@Senexis
Copy link
Contributor

Senexis commented May 24, 2018

Yep, which is why the PR exists, to attempt to fix it. :-)

The only reason this is happening is because the transients never had an "_id" field.

@atomicptr
Copy link
Member Author

Fixed with #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants