Conversation
pushfoo
left a comment
There was a problem hiding this comment.
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/unitor an appropriate subfolder inside it - Functions starting with
test_in those folders get run as tests - Exceptions, including those caused by
assertreceiving 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 |
pushfoo
left a comment
There was a problem hiding this comment.
Renaming the file was a good choice. 👍
tl;dr of the changes requested:
- Two of the lines seem to be missing
assertstatements - It would be a good idea to try
spatial_hash=Truewhen adding the wall SpriteList - Misc style cleanup issues
Co-authored-by: Paul <36696816+pushfoo@users.noreply.github.com>
pushfoo
left a comment
There was a problem hiding this comment.
Looks good after them most recent changes. 👍
Add dunder methods bool and contains.
Goes off: #1651