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.

Sign up for GitHub

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

Jump to bottom

Incorrect test for binary mode in _DownloadReportCheckFormat #152

Closed
parander opened this issue Sep 30, 2016 · 2 comments
Closed

Incorrect test for binary mode in _DownloadReportCheckFormat #152

parander opened this issue Sep 30, 2016 · 2 comments

Comments

@parander
Copy link

parander commented Sep 30, 2016 •
edited

in adwords.py the method _DownloadReportCheckFormat was recently changed. The new test for is_binary_mode is probably incorrect:

  def _DownloadReportCheckFormat(self, file_format, output):
    is_binary_mode = getattr(output, 'mode', 'w') == 'wb'
    if (file_format.startswith('GZIPPED_')
        and not (is_binary_mode or type(output) is io.BytesIO)):
      raise googleads.errors.GoogleAdsValueError('Need to specify a binary'
                                                 ' output for GZIPPED formats.')

This will incorrectly raise an exception for some valid binary mode combinations (e.g r+b, w+b).
@msaniscalchi
Copy link
Contributor

Hello,

Thanks for the report! It looks like this changed back in July. I don't think we expected the use-case to simultaneously read and write from the downloaded report file during an earlier refactoring, so the logic was simplified to what you see above. That said, I don't see a need for us to restrict this usage, so I think it makes sense to go back and update this. It probably won't make it into the update today, but we'll get around to it soon.

Regards,
Mark

@parander
Copy link
Author

parander commented Oct 3, 2016

I'm actually using a tempfile stream for this, but the 'wb' requirement makes a tempfile useless since it can never be read ;)

@parander parander mentioned this issue Oct 21, 2016
Closed
@pyup-bot pyup-bot mentioned this issue Dec 7, 2016
Closed
@pyup-bot pyup-bot mentioned this issue Jan 13, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@msaniscalchi @parander