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

Add support for the Legacy Bounty Hunter minigame #76

Merged
merged 2 commits into from
May 26, 2023

Conversation

dmeredith96
Copy link
Contributor

@dmeredith96 dmeredith96 commented May 24, 2023

This PR adds support for the Legacy Bounty Hunter minigame and fixes the stat fetching part of the API. INVALID_FORMAT_ERROR will no longer be thrown.

Tests were also updated.

Fixes issue #75

@evanw555
Copy link

Thanks for taking the time to address the API change, however I'd strongly urge you to consider the compatibility implications of this approach. By creating new keys (hunterLegacyBH, rogueLegacyBH) for the "legacy" activities and reassigning the old keys (hunterBH, rogueBH) to the new activities, you will likely cause data corruption in any applications using this library.

For instance, my application tracks user skill/boss data and saves it to a Postgres database using the keys defined in this library. If it were to track BH activity data as well, then adopting this change would cause all the existing data to be incorrectly interpreted as "new" bounty hunter data unless all the data is manually migrated from the old format to the new format.

It would be much safer to keep the old keys and instead create new keys for the new activities e.g. hunterBH2. I know the naming wouldn't be ideal, but this is a best-practice approach.

@dmeredith96
Copy link
Contributor Author

Thanks for taking the time to address the API change, however I'd strongly urge you to consider the compatibility implications of this approach. By creating new keys (hunterLegacyBH, rogueLegacyBH) for the "legacy" activities and reassigning the old keys (hunterBH, rogueBH) to the new activities, you will likely cause data corruption in any applications using this library.

For instance, my application tracks user skill/boss data and saves it to a Postgres database using the keys defined in this library. If it were to track BH activity data as well, then adopting this change would cause all the existing data to be incorrectly interpreted as "new" bounty hunter data unless all the data is manually migrated from the old format to the new format.

It would be much safer to keep the old keys and instead create new keys for the new activities e.g. hunterBH2. I know the naming wouldn't be ideal, but this is a best-practice approach.

Fair call out, can make that change tonight.

@evanw555
Copy link

The new naming looks good to me. Hopefully we can get this approved and merged soon, my application has been down for a few days and users are getting anxious

@maxswa
Copy link
Owner

maxswa commented May 26, 2023

Apologize for the delay! I'll be publishing this ASAP

@maxswa maxswa merged commit 99ad5eb into maxswa:main May 26, 2023
1 check 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