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

Total info showing incorrect XP #46

Open
jackbrown1993 opened this issue Nov 10, 2023 · 7 comments
Open

Total info showing incorrect XP #46

jackbrown1993 opened this issue Nov 10, 2023 · 7 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@jackbrown1993
Copy link

I will raise a PR to fix this. Total is showing 160973243\n224935 for XP instead of just 160973243

print (subset['total'])
{'rank': '199457', 'level': '2058', 'experience': '160973243\n224935'}
@jackbrown1993
Copy link
Author

print("Current total xp:", user.skill('total', 'experience'))

output:

Current total xp: 160973243
224935

@Coffee-fueled-deadlines
Copy link
Owner

Yeah it would appear that total is bleeding over into attack. Nice catch.

@Coffee-fueled-deadlines
Copy link
Owner

you're doing good as far as spotting errors go. I look forward to seeing your pull requests come through just make sure you're doing them and working from the correct branch. dev -> dev

@jackbrown1993
Copy link
Author

jackbrown1993 commented Nov 11, 2023

I think that adding 'total' to the skills list is the better option here, it prevents the need to use workaround '.splits' and should also ensure it's included in tests, the tests would have picked up that the total XP was a string.

As 'total' shows alongside all other skills on the OSRS hiscores and also has the same attributes (level, rank, xp) adding it as a skill keeps it consistent. Are you against this approach @Coffee-fueled-deadlines ? I will submit a draft PR with my proposed changes.

@jackbrown1993
Copy link
Author

jackbrown1993 commented Nov 11, 2023

If 'total' was added as a skill it would have been evaluated within pytest and pytest would have failed, although this is mute as adding 'total' to the skill list actually fixes the issue anyway.

If you really do not want 'total' added to the skill list, I can raise a PR to add the '.split' and also the below command to pytest which would have caught the issue.

assert type(int(boss_user.skill("total", "experience"))) == int, "Failed"

>       assert type(int(boss_user.skill("total", "experience"))) == int, "Failed"
E       ValueError: invalid literal for int() with base 10: '2135781666\n136'

OSRSBytes/tests/hiscores_test.py:8: ValueError

@jackbrown1993
Copy link
Author

#53 raised which I believe is the best way to fix this.

@Coffee-fueled-deadlines
Copy link
Owner

I'm not necessarily against it by any means. I believe my logic when I set it up was separating it out since it didn't quite "fit". Yet that was quite awhile ago. Let me look through your PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants