Skip to content

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

Merged
merged 10 commits into from
May 18, 2023
Merged

Improve scene.py #1748

merged 10 commits into from
May 18, 2023

Conversation

gran4
Copy link
Contributor

@gran4 gran4 commented May 5, 2023

Add dunder methods bool and contains.
Goes off: #1651

@gran4 gran4 changed the title Add duner methods bool and contains Improve scene.py May 5, 2023
Copy link
Member

@pushfoo pushfoo left a 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:

  1. Use the other files in that folder as test style reference, aside from the spacing
  2. 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

  1. Tests of a specific part of an arcade component should go in tests/unit or an appropriate subfolder inside it
  2. Functions starting with test_ in those folders get run as tests
  3. Exceptions, including those caused by assert receiving False, cause a test to fail
  4. Keep the style simple and beginner-friendly
  5. 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>
@gran4
Copy link
Contributor Author

gran4 commented May 6, 2023

I'll try to base the tests off of other ones

Copy link
Member

@pushfoo pushfoo left a 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:

  1. Two of the lines seem to be missing assert statements
  2. It would be a good idea to try spatial_hash=True when adding the wall SpriteList
  3. Misc style cleanup issues

Copy link
Member

@pushfoo pushfoo left a 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. 👍

@einarf einarf merged commit 9b8d548 into pythonarcade:development May 18, 2023
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