Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Quantum Chinese Chess] Add save_snapshot() and restore_last_snapshot() to QuantumWorld #173
[Quantum Chinese Chess] Add save_snapshot() and restore_last_snapshot() to QuantumWorld #173
Changes from 5 commits
0320380
f6e3aab
9c8fac2
ac18634
69a8a11
3903474
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want a better description here. Is the idea that we save a snapshot every move?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Doug, yes for chess we default to save a snapshot after each player's move, so later on if they choose to undo those quantum properties could be restored. I'm adding this into the comment. (In the end it's up to the game developer to decide in what granularity they allow the player to undo.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally these asserts should also have some sort a message (second argument to assert) to help callers debug issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most cases here are kind of self explanatory and would be a bit redundant to add messages. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is a specific exception you can capture here instead of the blanked except?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the possible error is only ValueError. But to be future proof I think maybe we could just include all kinds of exceptions here, instead of
try:
abc
except ValueError:
return False
except:
return False
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't think it's a good idea to catch all exceptions. Some you might not want to catch, like "KeyboardInterrupt" for instance. This is also going to swallow errors and hide them which will make debugging harder too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Doug. I'm updating the code to only catch ValueError. Other errors will be raised instead of swallowed.