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

feat!: Throw errors on missing inputs #7969

Merged
merged 1 commit into from
Mar 28, 2024
Merged

feat!: Throw errors on missing inputs #7969

merged 1 commit into from
Mar 28, 2024

Conversation

NeilFraser
Copy link
Contributor

@NeilFraser NeilFraser commented Mar 28, 2024

Previously generators could generate code from inputs that didn't exist and get back the empty string. This silent failure was causing problems for diagnosing issues. This PR changes the behaviour so that an error is thrown.

This will break generators which rely on the previous behaviour. Several of our demo blocks needed editing to accommodate this change.

Resolves #7665

Breaking Change

Calling statementToCode or valueToCode on inputs that don't exist will now throw an error instead of returning an empty string. If you have a block-code generator function that was intentionally doing this and relying on the old behavior, you should update it to check for the existence of the input before calling the toCode method. If you have a block-code generator that was unintentionally doing this (because you typoed the input name, for example), it will now be easier to spot your error.

Previously generators could generate code from inputs that didn't exist and get back the empty string.  This silent failure was causing problems for diagnosing issues.  This PR changes the behaviour so that an error is thrown.

This will break generators which rely on the previous behaviour.  Several of our demo blocks needed editing to accomodate this change.

Resolves #7665
@NeilFraser NeilFraser requested a review from a team as a code owner March 28, 2024 18:13
@github-actions github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: feature Adds a feature labels Mar 28, 2024
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Generally LGTM! But I'm not groking the changes to the text files, so going to wait to approve until I know what that's doing :P

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an unrelated change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it's an example of the sort of block this PR will break. This block mutates, sometimes it has an 'AT' input, sometimes not. Previously the generator would just get '' when generating from an input that doesn't exist. Now it throws an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok I see, the call to getAdjusted has been wrapped in the else so it only errors in the case where we do really expect there to be an input but it doesn't exist. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in the case of the dropdown being 'LAST' there is no 'AT' input, in the case of 'FROM_END' there is an 'AT' input. Generators can no longer just try generating from an input that may or may not exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an unrelated change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, same as earlier change. Same block, same input (AT), different language, and completely different code path.

Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Assuming my understanding is now correct, I'm going to go ahead and approve!

Didn't check if there are other blocks relying on the old behavior, so I'm trusting you caught all of 'em.

@NeilFraser
Copy link
Contributor Author

Didn't check if there are other blocks relying on the old behavior, so I'm trusting you caught all of 'em.

These are all the ones I found. But be aware that every generator (language) is different. I can't completely guarantee that I caught every one. And of course this will break everyone out there who has similar generators on blocks that mutate.

@NeilFraser NeilFraser merged commit 5a6f22f into develop Mar 28, 2024
13 checks passed
@NeilFraser NeilFraser deleted the fraser-nocode branch March 28, 2024 22:54
@github-actions github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: feature Adds a feature and removed PR: feature Adds a feature breaking change Used to mark a PR or issue that changes our public APIs. labels Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Used to mark a PR or issue that changes our public APIs. PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CodeGenerator.prototype.statementToCode and valueToCode should throw when given an invalid input name
2 participants