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

magicbot needs to inject before the constructor, or provide an initialization function #21

Open
virtuald opened this issue Apr 8, 2016 · 8 comments
Labels
magicbot magicbot package

Comments

@virtuald
Copy link
Member

virtuald commented Apr 8, 2016

If this isn't done, then it makes it very difficult to do one-time initialization that involves other components (such as motors), though arguably it should be done in robot.py?

@CarterFendley
Copy link

The feature would be useful. Here is my use case

Use case: I have a SwerveDrive class that coordinates four SwerveModules. I would like to add them to a dictionary with labels rr_motor, rl_motor, fr_motor, fl_motor this would make it much clearer when writing code. However currently am unable to do this in the constructor because the modules have not been injected yet.

@CarterFendley
Copy link

The feature would be useful. Here is my use case

I fixed this by using the setup() function from magic components.

@virtuald
Copy link
Member Author

One solution to this that occurs to me is that when magicbot is creating an instance, it could create at runtime a subclass of the instance class, and override __init__ with a custom constructor that would perform magicbot injections. This would allow components to have constructors that can access the injected variables.

@auscompgeek
Copy link
Member

auscompgeek commented Feb 12, 2019

Here's an crazy idea: we do DI on the constructor arguments. i.e. instead of this:

class Component:
    spam: Egg
    ham: Egg

One would have this:

class Component:
    def __init__(self, spam: Egg, ham: Egg):
        ...

The user would then be responsible for setting the attributes in the constructor themselves.

Obvious caveat here is that you can't have cyclic dependencies like this, but I suppose you really shouldn't in the first place?

@virtuald
Copy link
Member Author

That's a neat idea, but adds boilerplate because the user would need to set the attributes themselves.

@virtuald
Copy link
Member Author

Can't believe I never thought of this until now, but it seems you can call __new__ before calling __init__... soo we could do that? Not sure what terrible ramifications there would be to doing thing (presumably cause potential ordering problems if objects tried to use other non-initialized objects?)

class X:
    def __init__(self):
        print("init")


x = X.__new__(X)
print("new", x)
X.__init__(x)
print(x)

Prints out

new <__main__.X object at 0x10c4b5f28>
init
<__main__.X object at 0x10c4b5f28>

@auscompgeek auscompgeek added the magicbot magicbot package label Sep 29, 2019
@auscompgeek
Copy link
Member

Looking back at this, I'm still a fan of the DI on __init__ args idea. From the user perspective, it's simple and possibly easier to understand than what we do now.

adds boilerplate because the user would need to set the attributes themselves

Good thing dataclasses mean the boilerplate isn't a problem 😉

@auscompgeek
Copy link
Member

With #204 we now do dependency injection when instantiating components, in addition to the existing behaviour. It still needs to be documented though.

We may also want to reverse the order in which components are instantiated to make it more useful, although that may be confusing when looking at startup logs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
magicbot magicbot package
Projects
None yet
Development

No branches or pull requests

3 participants