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

Hanabi #71

Merged
merged 6 commits into from
May 10, 2020
Merged

Hanabi #71

merged 6 commits into from
May 10, 2020

Conversation

dissendahl
Copy link

Removed git submodule setup, added good default params, deleted typing hints.

@jkterry1
Copy link
Member

@weepingwillowben are we happy?

@benblack769
Copy link
Contributor

Please revert all the changes made to the API tests and the AECEnv classes. And then get it to pass the API tests in their original form.

@dissendahl
Copy link
Author

I kindly refuse to do so.

Changes done to AECenv and API tests result from conflicts with EnvLogger. Please either document what EnvLogger is doing or make expectations on child env functionality clear (e.g. you can use comments or partially capsule logging in the AECEnv directly with private functions) for others.

As I can observe ( 76694b5 ), EnvLogger is under active development resulting in adjustments to multiple envs, so I wont jump in adjusting, when things will change again eventually. I suggest refactoring the logging functionality.

Child envs should not break, when you add logging. If this happens, code orthogonality is violated. If there are necessary constraints, they should be documented. Tests alone are not a sufficient documentation of AECenv and related expectations about inheriting classes.

@benblack769
Copy link
Contributor

We understand that our development process right now is really screwed up and inefficient, mostly due to our inexperience with supporting the sorts of logging and exception features we are trying to implement with the EnvLogger.

So thanks for bringing it up. We shouldn't be so tolerant of sloppy development practices.

I'll start a discussion on some better ways of handling these errors consistently. The solution is probably a wrapper class plus environment registration or something similar. If you have other ideas, you can let me know.

@jkterry1 jkterry1 merged commit 66b2f49 into master May 10, 2020
@benblack769 benblack769 deleted the hanabi branch July 11, 2020 20:42
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.

4 participants