Develop #1389

Open
wants to merge 13 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

dmcritchie commented Mar 13, 2013

Further mturk changes since pending pull request from 9 months ago:

  1. Left build_list_params() function from boto/connection.py in its original version to avoid any possibility of disruption. Created a local, enhanced version of build_list_params() in boto/mturk/connection.py instead, to avoid any problems with other Amazon web services.
  2. Still includes changes from earlier pull request to support for mturk email notifications and its associated parsing, including sending test events.
  3. New mturk reject_qualification_request function.
  4. New mturk EmbeddedBinary class.
  5. Fixed bugs in mturk Application, JavaApplet, and Flash classes.

All of these changes have run on our production site and worked flawlessly.

Thanks,
Dennis McRitchie

Dennis McRitchie and others added some commits Feb 7, 2013

Modified build_list_params to support both an array of parameter name…
…s, or

a string containing a set of whitespace- or comma-delimited parameter names.
Formerly, only a single parameter name could be accepted as a string.

This change was submitted with a pull request on 11/27/2012, but was inadvertently removed, I believe, on 12/5 as part of fixing a regression with ec2.
As of September 2012, Amazon modified the 2012-03-25 API such that it…
… has now deprecated the REST and SOAP notification protocols; and to inform us of that fact they are no longer signing their REST and SOAP notification messages. The signature keyword now instead returns:

    Signature: u'DEPRECATED'
So we are now always returning True in this case so as not to break existing code.
boto/connection.py
if isinstance(items, basestring):
- items = [items]
+ items = re.findall('[^,\s]+', items)
@toastdriven

toastdriven Apr 3, 2013

Contributor

Maybe I'm missing something, but couldn't this just be items = items.replace(' ', ',').split(',')? That should be faster than a regex & clearer to boot. Don't get me wrong, I do love a good regex, just worrying about readability.

@dmcritchie

dmcritchie Apr 8, 2013

Contributor

Your suggestion would work for many cases, but for example:

items = ‘1, 2, 3’ (using comma-space delimiters for readability)

yields:

items
['1', '', '2', '', '3']

Similarly, if someone were using space delimiters and accidentally specified 2 consecutive spaces, an empty string would be produced. Empty strings might cause a problem with MTurk.

Also, my approach allows for any whitespace delimiters; not just spaces and commas. Perhaps the answer is to add an explanatory comment for the regex:

convert string to array; if more than one token, support comma or whitespace separators.

+ # If SetHITTypeNotification, specify the HIT type
+ if not test_event_type:
+ params = {'HITTypeId': hit_type}
+ # else if test notification, specify the notification type to be tested
@toastdriven

toastdriven Apr 3, 2013

Contributor

Looks vestigial. Can this be removed?

@dmcritchie

dmcritchie Apr 5, 2013

Contributor

I’m not sure what you’re suggesting is vestigial. If you meant the comment line, it is not vestigial, but is there (along with its partner 3 lines earlier) to document that the various ‘send test event’ calls need a TestEventType parameter, but do NOT need a HITTypeId parameter. The reverse holds true for the ‘set notification’ calls. If this is not what you meant, please let me know.

From: Daniel Lindsley [mailto:notifications@github.com]
Sent: Wednesday, April 03, 2013 5:45 PM
To: boto/boto
Cc: Dennis McRitchie
Subject: Re: [boto] Develop (#1389)

In boto/mturk/connection.py:

     """
  •    params = {'HITTypeId': hit_type}
    
  •    # If SetHITTypeNotification, specify the HIT type
    
  •    if not test_event_type:
    
  •        params = {'HITTypeId': hit_type}
    
  •    # else if test notification, specify the notification type to be tested
    

Looks vestigial. Can this be removed?


Reply to this email directly or view it on GitHubhttps://github.com/boto/boto/pull/1389/files#r3645850.

Contributor

toastdriven commented Apr 3, 2013

Any chance of getting some tests on this, please? Lots of logic changes & if we've already dropped it once, I'd like to assure there are more regressions in the future.

Contributor

dmcritchie commented Apr 8, 2013

Do you mean tests on all the submitted code? If so, almost all of it is notification code, which requires an infrastructure to be in place to create hits, set a (REST or email) notification on them, and then have a web server or mail server configured to receive those notifications. Not something that can easily be set up as a unit test suite. But I can attest to the fact that this code is all being used in our beta environment, and so is getting a reasonable amount of testing.

If you are referring to the one-line boto/connection.py change (which is the change that was previously dropped), then I can try to put something together for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment