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

feat: add checking for older GIDs #496

Merged
merged 35 commits into from
Jan 22, 2024

Conversation

podrivo
Copy link
Contributor

@podrivo podrivo commented Jan 21, 2024

This PR adds a new property games.extra.old_id, that is being used to add older game type IDs into the game-resolver.js. This makes sure that the new naming convention doesn't brake third party products that rely on gamedig.

It has a similar implementation as this PR (#487), but has a different essence on closing the gap about new/old GIDs, as discussed on #493 and #374.

Adds the option to skip this check:

  • skipOldIDs (defaults to false). Query will skip checking for older game type IDs.

I've included a tool for finding duplicates, that checks for new and old ids, and can be used in the CI. I'm not sure how to add this in the CI or if the code will work in the CI. Let me know and I'll update it!

tools/find_id_duplicates.js

Updates README.md, the Naming section on CONTRIBUTING.md and tweaks details on GAMES_LIST.md.
Adds a new file MIGRATION.md with a list of IDs from v4 to v5. The list was generated using tools/find-id-changes.js

@podrivo podrivo marked this pull request as ready for review January 21, 2024 11:35
@podrivo
Copy link
Contributor Author

podrivo commented Jan 21, 2024

CI ID tests is failing. Should we take a look at that as well with this PR and make sure everything is aligned?
But I'm not sure how to update this... Looks like it needs the rust repo?

@CosminPerRam Do you mind explaining me how this works? Thank you!

It fails on game names that has special chars:

IL-2 Sturmovik
Halo Online (ElDewrito)
Terraria - TShock
James Bond 007: Nightfire

// Example
game_id: "il2sturmovik"
game_name: "2 Sturmovik"
expected_id: "twosturmovik"

And it's also failing on this game name, for some reason:

game_id: "cs2d"
game_name: "CS2D"
expected_id: "c2d"
Here's the complete error log:
IDFail {
thread 'main' panicked at crates/id-tests/src/main.rs:30:5:
assertion failed: failed.is_empty()
    game_id: "cs2d",
    game_name: "CS2D",
    expected_id: "c2d",
    rule_stack: [
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
        NumbersAreTheirOwnWord,
        MoreThanTwoWordsMakeAcronym,
    ],
}
IDFail {
    game_id: "eldewrito",
    game_name: "Halo Online ",
    expected_id: "haloonline",
    rule_stack: [
        TwoWordsOrLessUseFullWords,
    ],
}
IDFail {
    game_id: "il2sturmovik",
    game_name: "2 Sturmovik",
    expected_id: "twosturmovik",
    rule_stack: [
        IfModForQueriesProcessOnlyModName,
        IfFirstWordNumberNoDigits,
        TwoWordsOrLessUseFullWords,
    ],
}
IDFail {
    game_id: "il2sturmovik",
    game_name: "IL-2 Sturmovik",
    expected_id: "i2s",
    rule_stack: [
        MoreThanTwoWordsMakeAcronym,
    ],
}
IDFail {
    game_id: "terrariatshosck",
    game_name: " TShock",
    expected_id: "tshock",
    rule_stack: [
        IfModForQueriesProcessOnlyModName,
        TwoWordsOrLessUseFullWords,
    ],
}
IDFail {
    game_id: "terrariatshosck",
    game_name: "Terraria - TShock",
    expected_id: "terrariatshock",
    rule_stack: [
        TwoWordsOrLessUseFullWords,
    ],
}
IDFail {
    game_id: "jb007n",
    game_name: "James Bond 007: Nightfire",
    expected_id: "jb0n",
    rule_stack: [
        MoreThanTwoWordsMakeAcronym,
    ],
}
7 (2%) IDs didn't match naming rules

lib/game-resolver.js Outdated Show resolved Hide resolved
tools/generate_games_list.js Outdated Show resolved Hide resolved
tools/find_id_duplicates.js Outdated Show resolved Hide resolved
@CosminPerRam
Copy link
Member

Do you mind explaining me how this works? Thank you!

@Douile made GIDs tests for rust-gamedig, the code is here.
While at it he also added support for testing node-gamedig gids (as we have talked that we would eventually also want to change gids in it).

It fails on game names that has special chars:

Well, testing gids is quite hard to do, and apparently not all cases are well-processed (those few left ones that are failing), that'll require some adjustments/fixes for the code.
Fixing CI and (if needed) the last few ids could come in a separate PR.

@Douile
Copy link
Contributor

Douile commented Jan 21, 2024

The ID test are based off the rules in CONTRIBUTING.md. They are only based of the ID and the name of each game so here it looks like we have the unfortunate situation of triggering a failure based on changes prior to this PR.

There are a few bugs shown here, I'll make an issue about them. The only actual bad ID looks like terraria which has a typo.

I don't think this test should block this PR though as the ID issues are not caused by it.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@podrivo
Copy link
Contributor Author

podrivo commented Jan 21, 2024

MIGRATION.md document was created. Please, let me know what else should I include there! 🙌

@podrivo
Copy link
Contributor Author

podrivo commented Jan 21, 2024

After a battle to roll back the file, I've managed to do it. Sorry about the mess...

@CosminPerRam
Copy link
Member

Thanks for taking care of this, looks good to merge.

skipOldIDs (defaults to false).

I would change this to true by default as the old ids aren't present in the GAMES_LIST file and would create a bit of confusing (and again, we wouldn't want users to use the old ones), this (the mention to use that option when handling old ids) can be mentioned in the first lines of the migration doc. Thoughts?

@podrivo
Copy link
Contributor Author

podrivo commented Jan 22, 2024

I would change this to true by default [...]

If we do this, it'll break implementations in Homepage or Uptime Kuma, right?
By checking old ids by default, we make sure Homepage or Uptime Kuma still functions normally.

@CosminPerRam
Copy link
Member

CosminPerRam commented Jan 22, 2024

Yes, but as seen in Homepage#2474, Homepage#1939, Homepage#1886 where I updated them, (Uptime Kuma was done by someone else), the person that updates any dependency is expected to test and mention any breaking changes that could occur by updating.
In these cases we would mention this and disable the skipping of old ids because of this, for the sake of avoiding creating any breaking changes for them.

@podrivo
Copy link
Contributor Author

podrivo commented Jan 22, 2024

Hmm, I thought you said that using new ids as default was not possible: #493 (comment)
I'm not against changing, just a bit confused. 😅

If skip checking old ids (skipOldIDs) is going to be true by default, maybe we should rename it to checkOldIDs?
So it's false by default.

// Don't check by default
gamedig --type counterstrike2 45.144.155.107:27015

// Check old ids
gamedig --type counterstrike2 45.144.155.107:27015 --checkOldIDs

@CosminPerRam
Copy link
Member

CosminPerRam commented Jan 22, 2024

Well I can't say that I'm explaining stuff very well in general, sorry for the confusion.

maybe we should rename it to checkOldIDs

LGTM.

Regarding that comment:

[...] so we should resolve it as smoothly as possible for them.

In this case, we should inform them that they need that option (the real issue would have been if we would have disabled old gids completely).

@podrivo
Copy link
Contributor Author

podrivo commented Jan 22, 2024

Should we also mention the migration document in our README?

@CosminPerRam
Copy link
Member

Yes

@CosminPerRam CosminPerRam merged commit 1f10ad0 into gamedig:master Jan 22, 2024
4 of 5 checks passed
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

Successfully merging this pull request may close these issues.

None yet

3 participants