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

fixes #22: throw a TypeError when padding is not an Object #24

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

michaelficarra
Copy link
Member

Fixes #22. /cc @bakkot

@bakkot
Copy link
Collaborator

bakkot commented Jun 6, 2024

Probably the appropriate place to perform this check is immediately after reading it? We generally validate early, when doing validation.

Either this is an API which behaves like most existing APIs in allowing strings as iterables, or is a new API which rejects strings as iterables. If it's allowing them, it should allow them for all of iterables, paddingOption, and the iterators being zipped. If it's rejecting them, it should reject them for all three.

Prior to this PR they were rejected for iterables but allowed for paddingOption and the iterators being zipped. After this PR they are rejected for iterables and paddingOption but still allowed in the iterators being zipped.

@michaelficarra
Copy link
Member Author

I'm not sure whether we want to permit iterables to yield strings. I think I'll open a PR and run it by committee.

@bakkot
Copy link
Collaborator

bakkot commented Jun 6, 2024

Approved but I still think the appropriate place to perform this check is immediately after reading _paddingOption_.

@michaelficarra
Copy link
Member Author

I can do that.

@@ -23,6 +23,7 @@ copyright: false
1. If _longest_ is *true*, then
1. Set _mode_ to ~longest~.
1. Set _paddingOption_ to ? Get(_options_, *"padding"*).
1. If _paddingOption_ is not *undefined* and _paddingOption_ is not an Object, throw a *TypeError* exception.
Copy link
Member

Choose a reason for hiding this comment

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

if longest isn't true, shouldn't this still validate paddingOption?

Copy link
Member Author

Choose a reason for hiding this comment

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

nah, we don't need to validate if we're not going to read it

Copy link
Member

Choose a reason for hiding this comment

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

In general (and in Temporal) I think we've been optimistically reading and validating every option whether they're used or not, to minimize depending on observable lookups later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If there's an option which is only relevant to one mode, it seems wasteful to read that option in other modes.

Copy link
Member Author

Choose a reason for hiding this comment

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

to minimize depending on observable lookups later

there won't be a later lookup because it's guaranteed to not be needed

@michaelficarra michaelficarra merged commit 941e785 into main Jun 6, 2024
1 check passed
@michaelficarra michaelficarra deleted the GH-22-again branch June 6, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reject non-Object padding
3 participants