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

Bugfix pokerstars hand history parser for cash game #41

Closed
wants to merge 27 commits into from

Conversation

chris060986
Copy link

As discussed here Issue-37 I started to fix the hand history parser for pokerstars. I think some further testing is needed, but may you can start with code review and may support me in testing.

Cash game hand samples are added.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 87.376% when pulling e419f98 on chris060986:bugfix/pokerstars into ac3f281 on pokerregion:master.

@chris060986
Copy link
Author

@kissgyorgy
Do you have any suggestions or will you merge the PR and push the python library to PyPi?

@kissgyorgy
Copy link
Contributor

Hi!
Thanks for your contribution! I will cherry pick a couple of small patches, but as for the hand history format change, but I have a policy that "all hand histories after 2014.01.01." should work, see:
https://poker.readthedocs.io/en/latest/handhistory.html#about-hand-history-changes

So instead of changing the old hand histories format, would you add the new histories? Both should be parsed successfully.

@@ -37,7 +37,7 @@
long_description=Path("README.rst").read_text(),
classifiers=classifiers,
keywords="poker",
author="Kiss György",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do that.

@chris060986
Copy link
Author

Sorry for that change of youre name. I just need a fast fix while during building docker containers I had errors with the non-ascii char.
I now decided to cancel to pull request because I do not have the time to fix all the hand histories from 2014 to now. For my "project" I want to use the library in docker containers. So I build a base container 1 with lxml another one with your poker library on a commit base and third one where I add my code 3.
Feel free to use my changes or integrate it in your repo but in generall I suggest to externalize the parsing information into configuration. For example use regex injected via environment variables to parse the hand history. So that its possible to adapt to new formats just by changing config and without publishing. Also in a docker/k8s world this would be very nice and makes it easily possible to support older versions by just delivering default configs.

@chris060986 chris060986 closed this Jul 2, 2020
@kissgyorgy
Copy link
Contributor

in generall I suggest to externalize the parsing information into configuration. For example use regex injected via environment variables to parse the hand history. So that its possible to adapt to new formats just by changing config and without publishing.

This would mean the user of this library should be able to understand the exact hand history format. The whole point of this library is to include this knowledge, so it's easy to parse hand histories. I understand this would be much effort though!

Thank for your contribution anyway, I will try to cherry-pick some of your ideas.

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.

3 participants