Skip to content
This repository has been archived by the owner on Oct 15, 2022. It is now read-only.

Meta::Helper: be more calling-context aware. #91

Merged
merged 2 commits into from
Aug 20, 2014
Merged

Meta::Helper: be more calling-context aware. #91

merged 2 commits into from
Aug 20, 2014

Conversation

mwmiller
Copy link
Contributor

Since we're offering html_enc as an alternative to encode_entities
it should behave in the same way when supplied a scalar and expecting
scalar output. Mostly we're concerned about when they want scalar
output, so we'll just encode the first element off the supplied array if
that's how they've called us.

uri_esc is similarly updated to provide similar semantics.

cc: @mintsoft

Since we're offering `html_enc` as an alternative to `encode_entities`
it should behave in the same way when supplied a scalar and expecting
scalar output.  Mostly we're concerned about when they want scalar
output, so we'll just encode the first element off the supplied array if
that's how they've called us.

`uri_esc` is similarly updated to provide similar semantics.
@mwmiller
Copy link
Contributor Author

An alternative is to concatenate the string in the scalar case. I'm not too bothered going either way.

@mintsoft
Copy link
Collaborator

@mwmiller I think I'd expect it to concatenate if going from array to scalar context for situations like:

return "and your answers are: ".html_enc(@answers)

Edit: on reflection, isn't that useless anyway?! It'd be far better to just do html_enc(join " ",@answers)

Based on input from @mintsoft.

I can see how this makes more sense, so I'm on-board, too.
@mwmiller
Copy link
Contributor Author

@mintsoft I can make arguments both ways. I'm honestly not sure which way provides "less astonishment." I'm mostly worried about the conversion of the single element to scalar case, so I don't have a well-formed opinion.

Edit: Also, I went ahead and changed it based on your preedit comment, because I had no strong opinion.

@mintsoft
Copy link
Collaborator

@mwmiller on reflection this morning I think my original thoughts make the most sense. I'd be weird to pass an array into something and have it appear to randomly lose data by only using the first element of the array.

So I think it's now correct

@mwmiller
Copy link
Contributor Author

have it appear to randomly lose data

This was what made me decide to switch it based on your original comment. I figure at least it's obvious what happened with all of the data if you call it "incorrectly."

@moollaza
Copy link
Member

This is awesome guys, nicely done. 👍
Regarding your discussion, I think the current implementation is best and makes it most useful for the context we tend to use this function in. I guess this all might change though when we have the Goodie templates?

@mintsoft
Copy link
Collaborator

@moollaza very possibly; but I think it probably depends on how they're implemented

@mwmiller
Copy link
Contributor Author

@moollaza I don't know how it might change with Goodie templates, but I'm pretty deeply involved there, so I'll keep myself in the loop.:grin:

@@ -37,7 +37,7 @@ encodes entities to safely post random data on HTML output.

=cut

$stash->add_symbol('&html_enc', sub { map { encode_entities($_) } @_ });
$stash->add_symbol('&html_enc', sub { return (wantarray) ? map { encode_entities($_) } @_ : encode_entities(join '', @_) });
Copy link
Member

Choose a reason for hiding this comment

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

didn't know wantarray existed. Very cool

moollaza added a commit that referenced this pull request Aug 20, 2014
Meta::Helper: be more calling-context aware.
@moollaza moollaza merged commit 3954b7b into duckduckgo:master Aug 20, 2014
@mwmiller mwmiller deleted the array_context branch August 20, 2014 19:13
@kablamo
Copy link
Member

kablamo commented Aug 22, 2014

I believe wantarray is often (perhaps always?) astonishing in some context. This is the YAPC talk that convinced me. Its kind of a fun talk actually:

http://aaroncrane.co.uk/talks/calamitous_context/
https://www.youtube.com/watch?v=NbP0AtEaxBU

Its really lame of me to make a comment after this has been released and its not really a big deal anyway. I just thought I would throw this idea out to consider for the future.

@mwmiller
Copy link
Contributor Author

Its really lame of me to make a comment after this has been released

@kablamo No, it's actually a very good point! I don't think it's particularly dangerous or horrible in the proximate case, but we should always be wary of context-sensitive code. I think the reason I am more or less ok with it here is because it directly on the provided arguments, not some hidden state which would make it more difficult to reason about what's happening. I might well be wrong about how this is better. 😁

Thanks for the link to the talk. I found it quite interesting.

@mintsoft
Copy link
Collaborator

@mwmiller you mentioned the goodie templates before, is there a place where that work is being done? Is there anything I can do to assist?

@mwmiller
Copy link
Contributor Author

@mintsoft There's nothing much to see right at the moment. I have a working prototype, but everything interesting happens on the closed-source side of the house at the moment. I expect in the next week or two the changes will appear to leap fully-formed into a repo near you. At the point, I look forward to your help in identifying edges I have missed as we go about converting goodies. 😁

@mintsoft
Copy link
Collaborator

@mwmiller cool, just lemme know if I can be of any use 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants