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

Add dependency injection of Subsystems to commands #1275

Merged
merged 5 commits into from Aug 20, 2018

Conversation

Projects
None yet
5 participants
@msoucy
Contributor

msoucy commented Aug 18, 2018

Per discussion on #1262 this adds a single optional Subsystem parameter that is automatically marked as requires().

msoucy added some commits Aug 17, 2018

Add subsystem dependency injection to commands.
Does not add constructors for classes with which it won't make sense.
@msoucy

This comment has been minimized.

Contributor

msoucy commented Aug 18, 2018

Note that #1262 and this one can both build off each other - if one gets merged, the other should be updated to reflect that.

@msoucy msoucy changed the title from Add dependency injection of Subsystems to commands to [WIP] Add dependency injection of Subsystems to commands Aug 18, 2018

@msoucy msoucy force-pushed the msoucy:dependantcommands branch from c5cbf40 to 4d28536 Aug 18, 2018

@msoucy msoucy changed the title from [WIP] Add dependency injection of Subsystems to commands to Add dependency injection of Subsystems to commands Aug 18, 2018

if (timeout < 0) {
throw new IllegalArgumentException("Timeout must not be negative. Given:" + timeout);
}
m_timeout = timeout;

This comment has been minimized.

@auscompgeek

auscompgeek Aug 19, 2018

Contributor

Wouldn't it make more sense to delegate to this(name, timeout) here? (Same for the others.)

Command delegates constructors
Avoids duplicating logic with the timeout and name.
@PeterJohnson

This comment has been minimized.

Member

PeterJohnson commented Aug 20, 2018

@frcjenkins test this please

@PeterJohnson PeterJohnson merged commit e28295f into wpilibsuite:master Aug 20, 2018

7 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
frcjenkins - Athena Build finished.
Details
frcjenkins - Linux Build finished.
Details
frcjenkins - Mac Build finished.
Details
frcjenkins - Windows Build finished.
Details

@msoucy msoucy deleted the msoucy:dependantcommands branch Aug 21, 2018

@Oblarg

This comment has been minimized.

Oblarg commented Aug 26, 2018

Just a minor gripe about the name "requirement" - it seems ill-fitting if one intends to use the injected subsystem for more than just calling requires() (which one should, if one is going through the trouble of injecting it). Is there any reason not to just call it "subsystem?"

calcmogul added a commit to calcmogul/allwpilib that referenced this pull request Sep 1, 2018

Fix some PIDCommand constructors not forwarding subsystems
Also renamed argument from requirement to subsystem as per
wpilibsuite#1275 (comment).

calcmogul added a commit to calcmogul/allwpilib that referenced this pull request Sep 1, 2018

Fix some PIDCommand constructors not forwarding subsystems
Also added missing constructor to wpilibc's InstantCommand and renamed
argument from requirement to subsystem as per
wpilibsuite#1275 (comment).

calcmogul added a commit to calcmogul/allwpilib that referenced this pull request Sep 1, 2018

Fix some PIDCommand constructors not forwarding subsystems
Also added missing constructor to wpilibc's InstantCommand and renamed
argument from requirement to subsystem as per
wpilibsuite#1275 (comment).

PeterJohnson added a commit that referenced this pull request Sep 2, 2018

Fix some PIDCommand constructors not forwarding subsystems (#1299)
Also added missing constructor to wpilibc's InstantCommand and renamed
argument from requirement to subsystem as per
#1275 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment