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.

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

Fix bug where opening files in GCS forgets to define a name field #506

Merged
merged 5 commits into from
Jun 17, 2020

Conversation

todor-markov
Copy link
Contributor

@todor-markov todor-markov commented Jun 5, 2020

Motivation

Fixes #505

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass

@mpenkov
Copy link
Collaborator

mpenkov commented Jun 6, 2020

@todor-markov Thank you for making the PR! Can you add a unit test that checks for your new functionality?

@bsikander
Copy link

+1 for the PR. I have been messing with this for 2 hours now and this is the exact issue. Could we please speed up the process to get this in.

@bsikander
Copy link

Was it working in any version?

@mpenkov mpenkov self-assigned this Jun 16, 2020
@mpenkov
Copy link
Collaborator

mpenkov commented Jun 16, 2020

Was it working in any version?

Yes, because of this.

@bsikander
Copy link

I am new to smart-open, so, I don't understand why it was working before.

Btw, i don't understand the reason for these TODOs in the code. In added logging to my code to find the problem and got TODO multiple times. Was not helpful.
https://github.com/RaRe-Technologies/smart_open/blob/0a3619144a75ea002304c487b8f8a9ee2c867c33/smart_open/smart_open_lib.py#L403

@bsikander
Copy link

Thank you for adding the tests.

@mpenkov
Copy link
Collaborator

mpenkov commented Jun 17, 2020

I am new to smart-open, so, I don't understand why it was working before.

This PR adds .name attribute to file objects created by the GCS backend. Previously, this attribute was missing. However, the code that relied on the .name attribute was robust enough to handle its absense.

Btw, i don't understand the reason for these TODOs in the code. In added logging to my code to find the problem and got TODO multiple times.

TODOs serve as a reminder to developers about parts of the code that are incomplete (or require attention for whatever reason). They are not ideal - in retrospect, it would have been better to fix the underlying problem, or create a github ticket to track the issue. Hindsight is always 20/20.

Was not helpful.

You're welcome to make PRs to make things more helpful.

@mpenkov mpenkov merged commit bac9ab0 into piskvorky:develop Jun 17, 2020
@mpenkov
Copy link
Collaborator

mpenkov commented Jun 17, 2020

Thank you @todor-markov ! Congratulations on your first contribution to smart_open 🥇

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

Successfully merging this pull request may close these issues.

Opening GCS files via smart_open.open fails to add the name field to the file object
3 participants