Skip to content
This repository has been archived by the owner on Dec 21, 2017. It is now read-only.

Encode href params before substituting them into the url #12

Merged
merged 1 commit into from
Oct 17, 2014

Conversation

petehamilton
Copy link
Contributor

@greysteil This fixes https://exceptions.gocardless.com/production/gocardless-live/group/5303/

Review?

Next steps:

  1. Bump Atum
  2. Update Atum dependency in GCER

@greysteil
Copy link
Contributor

👍

@@ -58,6 +58,8 @@ def construct_path(*params)
"for #{expected_params.count}"
end

params.map! { |param| URI.escape(param) }

Choose a reason for hiding this comment

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

Not saying you should write this differently (in fact, you totally should not), but purely for a "oh this is cool, out of interest", you could write this like so:

params.map!(&URI.method(:escape))

Was curious to see if it would work and apparently it does. Yay Ruby.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, didn't think the & map syntax worked/worked nicely for that kind of
call! TIL...

On Thursday, 16 October 2014, Jack Franklin notifications@github.com
wrote:

In lib/atum/core/schema/link_schema.rb:

@@ -58,6 +58,8 @@ def construct_path(*params)
"for #{expected_params.count}"
end

  •      params.map! { |param| URI.escape(param) }
    

Not saying you should write this differently (in fact, you totally should
not), but purely for a "oh this is cool, out of interest", you could write
this like so:

params.map!(&URI.method(:escape))

Was curious to see if it would work and apparently it does. Yay Ruby.


Reply to this email directly or view it on GitHub
https://github.com/gocardless/atum/pull/12/files#r18985588.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jackfranklin For now I'll probably leave it, bit clearer?

Choose a reason for hiding this comment

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

Oh yeah, sorry, I think that you should leave it, yours is much clearer!

petehamilton added a commit that referenced this pull request Oct 17, 2014
Encode href params before substituting them into the url
@petehamilton petehamilton merged commit d07c1f3 into master Oct 17, 2014
@petehamilton petehamilton deleted the url-encode-href-params branch October 17, 2014 09:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants