search

tl-its-umich-edu / canvas

Integration scripts between ITS TL and Instructure Canvas
Other
3 stars 8 forks source link

TLUNIZIN-591 used Addressable::URI.escape instead of URI.escape #65

Closed zqian closed 9 years ago

dlhaines commented 9 years ago

Can the reason this matters be documented? Is it a good idea to recommend that others do the same?

zqian commented 9 years ago

Added comments as for why Addressable::URI.escape is selected to replace URI.encode

dlhaines commented 9 years ago

Looks good.

lsloan commented 9 years ago

I see you've replaced a deprecated function call with an equivalent one. You've also changed the output format slightly and print the text of an exception (I presume that's what it is, since I don't know Ruby well). I take it that last point is the purpose behind the title of the JIRA issue, "handle warnings in Ruby verbose output with sis_upload.rb script". Is that correct?

zqian commented 9 years ago

Lance:

I have included the warning inside the TLUNIZIN-519 comment:

"/usr/local/ctools/app/ctupload/bin/sis_upload.rb:95:in `Canvas_API_IMPORT': warning: URI.escape is obsolete"

This is why I switch to another package in order to avoid this error.

lsloan commented 9 years ago

Got it. I didn't search for the JIRA issue before I reviewed the PR.

Lance E Sloan, Application Developer Univ. of Michigan, Info. and Tech. Services - http://www.its.umich.edu/ I'm definitely not endorsing specific products. Honest!

On Wed, Jul 1, 2015 at 2:22 PM, Zhen Qian notifications@github.com wrote:

Lance:

I have included the warning inside the TLUNIZIN-519 comment:

"/usr/local/ctools/app/ctupload/bin/sis_upload.rb:95:in `Canvas_API_IMPORT': warning: URI.escape is obsolete"

This is why I switch to another package in order to avoid this error.

— Reply to this email directly or view it on GitHub https://github.com/tl-its-umich-edu/canvas/pull/65#issuecomment-117783673 .

zqian commented 9 years ago

I should have included the description into the Pull Request. I will do it next time.

Thanks,

On Wed, Jul 1, 2015 at 2:30 PM, Lance E Sloan notifications@github.com wrote:

Got it. I didn't search for the JIRA issue before I reviewed the PR.

Lance E Sloan, Application Developer Univ. of Michigan, Info. and Tech. Services - http://www.its.umich.edu/ I'm definitely not endorsing specific products. Honest!

On Wed, Jul 1, 2015 at 2:22 PM, Zhen Qian notifications@github.com wrote:

Lance:

I have included the warning inside the TLUNIZIN-519 comment:

"/usr/local/ctools/app/ctupload/bin/sis_upload.rb:95:in `Canvas_API_IMPORT': warning: URI.escape is obsolete"

This is why I switch to another package in order to avoid this error.

— Reply to this email directly or view it on GitHub < https://github.com/tl-its-umich-edu/canvas/pull/65#issuecomment-117783673>

.

— Reply to this email directly or view it on GitHub https://github.com/tl-its-umich-edu/canvas/pull/65#issuecomment-117788827 .