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

Move things, break loops #316

Merged
merged 15 commits into from
Jul 19, 2019
Merged

Move things, break loops #316

merged 15 commits into from
Jul 19, 2019

Conversation

AstraLuma
Copy link
Member

@AstraLuma AstraLuma commented Jul 13, 2019

Grab a u-haul! 🚚

Generally moving things and reducing imports to improve the import loop situation

  • Break up "the machinery implementing a thing" and "a pile of things that use the thing"
  • Prefer module imports to individual imports, so any unavoidable loops don't wreck everything.

Depends on #306 because I didn't feel like dealing with the merge conflict.

No docs changes because this doesn't touch anything publicly documented.

@AstraLuma AstraLuma requested a review from a team as a code owner July 13, 2019 03:13
@AstraLuma
Copy link
Member Author

Note: I know this code is all pygame-specific, but the way I figure, separating it out like this should make it easier to bundle it up into ppb.systems.pygame and autodetect things later.

@AstraLuma AstraLuma mentioned this pull request Jul 14, 2019
@AstraLuma AstraLuma changed the title [WIP] Move things, break loops Move things, break loops Jul 14, 2019
@AstraLuma
Copy link
Member Author

Ok, I think this is ready to go as soon as #306 merges.

Copy link
Collaborator

@pathunstrom pathunstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one file I'd suggest a real change to, otherwise, this looks good.

ppb/eventlib.py Show resolved Hide resolved
import ppb.events as events
import ppb.keycodes as keys
from ppb.systems.base import System
from ppb.systems.renderer import DEFAULT_RESOLUTION
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking this, but we should probably move shared defaults to their own file somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly? If we had more than one?

I grouped it like this because resolution felt like a renderer concern, and the renderer is responsible for window management currently?

@AstraLuma AstraLuma mentioned this pull request Jul 14, 2019
@AstraLuma
Copy link
Member Author

@pathunstrom can you take a second pass on this?

Copy link
Collaborator

@pathunstrom pathunstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bors r+

@pathunstrom pathunstrom added the bors Someone has bors r+ this PR label Jul 18, 2019
bors bot added a commit that referenced this pull request Jul 18, 2019
316: Move things, break loops r=pathunstrom a=astronouth7303

Grab a u-haul! 🚚 

Generally moving things and reducing imports to improve the import loop situation

* Break up "the machinery implementing a thing" and "a pile of things that use the thing"
* Prefer module imports to individual imports, so any unavoidable loops don't wreck everything.

Depends on #306 because I didn't feel like dealing with the merge conflict.

No docs changes because this doesn't touch anything publicly documented.

Co-authored-by: Jamie Bliss <jamie@ivyleav.es>
@bors
Copy link
Contributor

bors bot commented Jul 18, 2019

Timed out

@pathunstrom
Copy link
Collaborator

bors retry

bors bot added a commit that referenced this pull request Jul 18, 2019
316: Move things, break loops r=pathunstrom a=astronouth7303

Grab a u-haul! 🚚 

Generally moving things and reducing imports to improve the import loop situation

* Break up "the machinery implementing a thing" and "a pile of things that use the thing"
* Prefer module imports to individual imports, so any unavoidable loops don't wreck everything.

Depends on #306 because I didn't feel like dealing with the merge conflict.

No docs changes because this doesn't touch anything publicly documented.

Co-authored-by: Jamie Bliss <jamie@ivyleav.es>
@bors
Copy link
Contributor

bors bot commented Jul 18, 2019

Timed out

@AstraLuma
Copy link
Member Author

bors retry

bors bot added a commit that referenced this pull request Jul 18, 2019
316: Move things, break loops r=pathunstrom a=astronouth7303

Grab a u-haul! 🚚 

Generally moving things and reducing imports to improve the import loop situation

* Break up "the machinery implementing a thing" and "a pile of things that use the thing"
* Prefer module imports to individual imports, so any unavoidable loops don't wreck everything.

Depends on #306 because I didn't feel like dealing with the merge conflict.

No docs changes because this doesn't touch anything publicly documented.

Co-authored-by: Jamie Bliss <jamie@ivyleav.es>
@bors
Copy link
Contributor

bors bot commented Jul 18, 2019

Timed out

@AstraLuma
Copy link
Member Author

bors retry

bors bot added a commit that referenced this pull request Jul 18, 2019
316: Move things, break loops r=pathunstrom a=astronouth7303

Grab a u-haul! 🚚 

Generally moving things and reducing imports to improve the import loop situation

* Break up "the machinery implementing a thing" and "a pile of things that use the thing"
* Prefer module imports to individual imports, so any unavoidable loops don't wreck everything.

Depends on #306 because I didn't feel like dealing with the merge conflict.

No docs changes because this doesn't touch anything publicly documented.

Co-authored-by: Jamie Bliss <jamie@ivyleav.es>
@bors
Copy link
Contributor

bors bot commented Jul 19, 2019

Timed out

@pathunstrom
Copy link
Collaborator

bors retry

bors bot added a commit that referenced this pull request Jul 19, 2019
316: Move things, break loops r=pathunstrom a=astronouth7303

Grab a u-haul! 🚚 

Generally moving things and reducing imports to improve the import loop situation

* Break up "the machinery implementing a thing" and "a pile of things that use the thing"
* Prefer module imports to individual imports, so any unavoidable loops don't wreck everything.

Depends on #306 because I didn't feel like dealing with the merge conflict.

No docs changes because this doesn't touch anything publicly documented.

Co-authored-by: Jamie Bliss <jamie@ivyleav.es>
@bors
Copy link
Contributor

bors bot commented Jul 19, 2019

Build succeeded

  • docs
  • Linux python:3.6-slim
  • Linux python:3.7-slim
  • macOS PYTHON:3.6.8
  • macOS PYTHON:3.7.2
  • pep517
  • Windows python:3.6-windowsservercore-1809
  • Windows python:3.7-windowsservercore-1809

@bors bors bot merged commit 9932158 into ppb:master Jul 19, 2019
bors bot added a commit that referenced this pull request Jul 23, 2019
315: Add AssetLoaded event r=pathunstrom a=astronouth7303

Fire an event when an asset finishes loading and becomes available.

Depends on #306, #316 

328: Manual Testing r=pathunstrom a=astronouth7303

Initial version of manual/visual tests.

Co-authored-by: Jamie Bliss <jamie@ivyleav.es>
@AstraLuma AstraLuma deleted the giant-move branch May 11, 2020 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bors Someone has bors r+ this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants