Skip to content

Simulation of Componentables will render components added after starting #602

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 3 commits into from
Feb 3, 2023

Conversation

angusjfw
Copy link
Contributor

@angusjfw angusjfw commented Jan 16, 2023

Status Ticket/Issue
Ready Ticket

Main changes

Other notes (e.g. implementation quirks, edge cases, questions / issues)

  • I had to make a fix to the Componentable class which maybe could be handled better. When using componentable.add_component with a 'name' parameter, the component's name is overridden with the parameter, presumably so that it is consistent with what the parent component will call it. But this did not override the name on the component's Recreatable config, so I had to change it to do so. Otherwise this meant that my sim code, which relies on the component names from the config to distinguish them, wouldn't add work with this code:
    pitop.add_component(Button("D1"))
    pitop.add_component(Button("D2"), name="button2")

because pitop.button2.config['name'] == 'button'
but did work with this

    pitop.add_component(Button("D1"))
    pitop.add_component(Button("D2", name="button2"))

So this works now but it feels not quite right messing with the recreatables _config... probably fine though.

Manual testing tips

  • I updated an example as well as adding a test

@angusjfw angusjfw force-pushed the simulation-add-component branch from e1303f2 to beb0b7a Compare January 17, 2023 13:27
@angusjfw angusjfw force-pushed the simulation-add-component branch from beb0b7a to c1bc884 Compare January 19, 2023 12:37
@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Base: 59.85% // Head: 59.81% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (488e1b4) compared to base (78dd869).
Patch coverage: 20.58% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
- Coverage   59.85%   59.81%   -0.04%     
==========================================
  Files         155      155              
  Lines        7811     7829      +18     
==========================================
+ Hits         4675     4683       +8     
- Misses       3136     3146      +10     
Flag Coverage Δ
unittests 59.81% <20.58%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/simulation/pitop/simulation/simsprite.py 40.90% <5.26%> (-0.76%) ⬇️
packages/simulation/pitop/simulation/simulation.py 48.06% <35.71%> (-1.08%) ⬇️
packages/core/pitop/core/mixins/componentable.py 75.43% <100.00%> (+2.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@angusjfw angusjfw merged commit 5270b38 into master Feb 3, 2023
@angusjfw angusjfw deleted the simulation-add-component branch February 3, 2023 11:16
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.

2 participants