Skip to content

Conversation

@BeksOmega
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes #6525

Proposed Changes

Refactors the procedures namespace to use the new procedure block interface methods instead of the old ones.

Should be no change in behavior.

Reason for Changes

Realized I actually did need to refactor this so that external devs don't have to implement the old getProcedureDef and getProcedureCall methods.

Test Coverage

Passes all existing tests.

Documentation

N/A

Additional Information

Dependent on #6736

@github-actions github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Jan 9, 2023
@BeksOmega BeksOmega mentioned this pull request Jan 9, 2023
4 tasks
@BeksOmega BeksOmega force-pushed the chore/refactor-procedures-namespace branch from 3a1391d to 4301abf Compare January 10, 2023 00:23
@BeksOmega BeksOmega marked this pull request as ready for review January 10, 2023 01:58
@BeksOmega BeksOmega requested a review from a team as a code owner January 10, 2023 01:58
@BeksOmega BeksOmega requested a review from gonfunko January 10, 2023 01:58
Copy link
Contributor

@gonfunko gonfunko left a comment

Choose a reason for hiding this comment

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

Do we want to mark this as a breaking change? I realize the root blocks are updated to accommodate it, but since the interface changed it might still be worth calling out?

@BeksOmega
Copy link
Contributor Author

All the changes I made should be backwards compatible (that was my goal anyway). Do you see something I missed?

@gonfunko
Copy link
Contributor

All the changes I made should be backwards compatible (that was my goal anyway). Do you see something I missed?

Just the change to the IProcedureBlock interface; theoretically somebody could have created a class that implements it and doesn't inherit from the core procedure blocks, in which case it would break I think? But I can easily be convinced that's enough of an edge case to not worry about.

@BeksOmega
Copy link
Contributor Author

All the changes I made should be backwards compatible (that was my goal anyway). Do you see something I missed?

Just the change to the IProcedureBlock interface; theoretically somebody could have created a class that implements it and doesn't inherit from the core procedure blocks, in which case it would break I think? But I can easily be convinced that's enough of an edge case to not worry about.

Ah sweet, that was only released in December, and according to our readme: "Once a new API is merged into master it is considered beta until the following release." so we get away with this one scot-free 😎

@BeksOmega BeksOmega merged commit 902c309 into RaspberryPiFoundation:develop Jan 11, 2023
@BeksOmega BeksOmega deleted the chore/refactor-procedures-namespace branch May 3, 2023 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: chore General chores (dependencies, typos, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor the Procedures namespace

2 participants