-
Notifications
You must be signed in to change notification settings - Fork 343
Improve scene.py #1748
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
Improve scene.py #1748
Conversation
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.
tl;dr looking good; I think we can probably merge this after adding some tests 👍
Current Changes
__bool__
will enhance code clarity in custom room / map loading views. Thank you for adding it. It's exactly what I'll need for a prototype at a later point.:+1:
We can also probably merge __contains__
, although I have some lingering out-of-scope API structure concerns. I'll try to get around to looking at Scene
more closely soon. If we don't think of something clearer or more expressive, this will probably do.
What to Add
Tests for should go in one or two appropriately named files in tests/unit/scene
. I'd do the following:
- Use the other files in that folder as test style reference, aside from the spacing
- If you want, make a separate PR to touch up the spacing style for the existing tests
tl;dr of what you need to know to write tests
- Tests of a specific part of an arcade component should go in
tests/unit
or an appropriate subfolder inside it - Functions starting with
test_
in those folders get run as tests - Exceptions, including those caused by
assert
receiving False, cause a test to fail - Keep the style simple and beginner-friendly
- Try not to go overboard on the pytest fixtures unless you need to; they can make reasoning about code much harder
I've started working on a more in-depth Contributing Guide, and will try to expand on the above in a dedicated section or page.
Co-authored-by: Paul <36696816+pushfoo@users.noreply.github.com>
I'll try to base the tests off of other ones |
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.
Renaming the file was a good choice. 👍
tl;dr of the changes requested:
- Two of the lines seem to be missing
assert
statements - It would be a good idea to try
spatial_hash=True
when adding the wall SpriteList - Misc style cleanup issues
Co-authored-by: Paul <36696816+pushfoo@users.noreply.github.com>
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.
Looks good after them most recent changes. 👍
Add dunder methods bool and contains.
Goes off: #1651