Skip to content

Remove BlueprintBuilder as a possible runner input type#98

Merged
adamziel merged 1 commit intoWordPress:trunkfrom
reimic:rm-builder-as-run-input
Apr 4, 2024
Merged

Remove BlueprintBuilder as a possible runner input type#98
adamziel merged 1 commit intoWordPress:trunkfrom
reimic:rm-builder-as-run-input

Conversation

@reimic
Copy link
Collaborator

@reimic reimic commented Apr 4, 2024

What does this PR do?

  • removes the possibility of parsing a Blueprint from a BlueprintBuilder

What problem does it fix?

  • the BlueprintParser's method parse could accept a BlueprintBuilder object as a potential input which could lead to errors

As context:

  • the only difference between passing a Blueprint and a BlueprintBuilder was that the method toBlueprint() was called for the builder, which has little utility
  • calling the method toBlueprint() on a specific instance of the BlueprintBuilder always returns the same instance of a Blueprint
  • this leads to an issue where someone:
    • creates a builder,
    • sets some of the variables,
    • runs the code with success,
    • sets some more variables,
    • runs it expecting another blueprint being generated and receives an error.
  • running the same Blueprint twice greatly confuses the program.

In conclusion:

  • the user expected the builder to generate another instance of a Blueprint, but it just set some values to the first instance. The runner extracted the first instance instead of creating a new one.

The explicit source of this particular error will be fixed in another PR, but as an example of what could go wrong:
InvalidArgumentException: Progress cannot go backwards (tried updating to 3.0105460023897 when it already was 3.0157410774586)

How to test if it works?

  • this PR only removes an execution path from the parser

@reimic reimic added the bug Something isn't working label Apr 4, 2024
@reimic reimic requested a review from adamziel April 4, 2024 15:00
@adamziel adamziel merged commit 87afea1 into WordPress:trunk Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants