Subsystems as robot class constructor arguments

[gnawinganimal]

I feel like it would be nice if subsystems could be passed as constructor parameters to the main robot class, just as a little usability shortcut. Then we could be all like

class Poe : Robot(
    Drivetrain,
    Lift,
    Intake
)

which would be nice

[gnawinganimal]

Oops, somehow my other commit got thrown in there

[andrewda]

Sorry for the noise, just changed base to take a peek at the actual diff.

Anyways, this is interesting. What benefit do you think this provides? Just more separation between actual robot code, I presume? And just to be clear, I assume it would change this file:

class Poe : Robot() {
    override fun onCreate() {
        Drivetrain
        Intake
        Elevator
        Climber
        Auto
        Modes
        Lights

        UDPServer.start()
        CameraServer.getInstance().startAutomaticCapture()

        initPreferences()
        logTelemetry()
    }
}

To this:

class Poe : Robot(
    Drivetrain
    Intake
    Elevator
    Climber
    Auto
    Modes
    Lights
) {
    override fun onCreate() {
        UDPServer.start()
        CameraServer.getInstance().startAutomaticCapture()

        initPreferences()
        logTelemetry()
    }
}

[gnawinganimal]

Hmm, wouldn't you have to manually add the subsystems to the LifecycleDistributor? This takes care of that

And yes, the more organized the code is, the better. If you need to call an object or field just to force it to initialize, it really shouldn't be lazy in the first place

[andrewda]

https://github.com/sertain/sertain/blob/6ccac283921bf7320d2505901946db7302349eb7/core/src/main/kotlin/org/sertain/command/Subsystem.kt#L16

They should be automatically added to the RobotLifecycle through the initSendable() method (called automatically when Subsystem is initialized). As far as I can tell, all this does is move the place where Subsystems are added to the lifecycle. Maybe I'm missing something, though.

[gnawinganimal]

I actually removed that in this commit, because adding it to the dashboard has nothing to do with adding a life-cycle listener.

No, you're not wrong. All it does is move the place, but in my opinion it makes more sense this way. Maybe we should ask some of the others opinions?

[andrewda]

Okay, let's try this along with your other PR on Friday. If it works then LGTM.

BTW can you rebase your other commit out of this PR? If the other PR is merged first it'll just cause conflicts we'll have to deal with later.

[gnawinganimal]

How would I do that? I tried on my local repo and I totally screwed the entire history over. Git confuses me ❓ 😕 ❓

[andrewda]

https://www.clock.co.uk/insight/deleting-a-git-commit

[gnawinganimal]

Tried what they suggested and it didn't work. I used git rebase --onto usability~6 usability~5 usability It did something, but definitely did not delete the commit from the branch Did I reference the wrong commits? I seriously have no idea how to tell

[andrewda]

Haha okay, I'll try to see what's up in a bit

[gnawinganimal]

I really just need to spend 5 hours just reading the git documentation 😆