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

Added _id return from save_file and kwargs to be stored in the document #114

Closed
wants to merge 4 commits into from

Conversation

achawkins
Copy link
Contributor

@achawkins achawkins commented Aug 1, 2018

Made some small changes to the save_file function. I added kwargs that will be placed into the document and returned the created document's _id.

@dcrosta
Copy link
Owner

dcrosta commented Aug 2, 2018

Thanks for this! Looks like there's still some style tweaks to work out. Don't sweat the other build failures, I'll run this locally to ensure everything passes.

Two things:

  1. Could you add a note to the docstring explaining what the extra kwargs do? (ie that they're passed through to PyMongo and become attributes of the document for the file)
  2. Could you add a test in https://github.com/dcrosta/flask-pymongo/blob/master/flask_pymongo/tests/test_gridfs.py ?

@dcrosta
Copy link
Owner

dcrosta commented Aug 2, 2018

For the style, it wants you to format long call sites like:

response = current_app.response_class(
    data,
    mimetype=fileobj.content_type,
    direct_passthrough=True,
)

and long function/method signatures like:

def we_dont_have_any_of_these_yet(
    self,
    foo,
    bar,
    baz=1,
):
    # first line of func...

You can run tox -e style locally to validate, which is a lot faster than going through Travis. pip install tox if you haven't already.

Thanks again!

@achawkins
Copy link
Contributor Author

Thanks for the feedback! I will make those changes.

@achawkins
Copy link
Contributor Author

The style looks good now but I am getting an E999 error with the tox run. Do you know what that is? I think it has to do with the version of python on my machine, but I am not certain. If it is a version problem, I assume it will pass in Travis.

@achawkins
Copy link
Contributor Author

Ah, I get it. Python 2 does not allow commas after **kwargs, but Python 3.6 does. So we pretty much have to get it down to a single line to pass the style checks.

I am just going to set the _id from storage.put() to a variable instead of directly returning it. This will put us less than the 90 column limit.

Sorry, this probably shouldn't have take so long to figure out.

@dcrosta
Copy link
Owner

dcrosta commented Aug 2, 2018

I’m on my phone so can’t look in detail now, but if there’s a way to disable that comma check after kwargs, I’d accept that change too. I haven’t run into that particular one with flake8 yet, but I’m sure someone on the internet has.

@achawkins
Copy link
Contributor Author

I was able to get it to fit within the required column limit, so the style check passed. Would you prefer the multi-line way? The last Travis run only threw an error on pip install tox tox-docker for the Python 3.4 checks.

@dcrosta
Copy link
Owner

dcrosta commented Aug 6, 2018

I think the 3.4 build errors were unelated. I've merged this (manually) and will fix anything that breaks in case I was wrong. Thanks for the contribution!

@dcrosta dcrosta closed this Aug 6, 2018
@achawkins
Copy link
Contributor Author

Sounds good; thanks!

dcrosta added a commit that referenced this pull request Aug 6, 2018
@dcrosta
Copy link
Owner

dcrosta commented Aug 6, 2018

https://pypi.org/project/Flask-PyMongo/2.1.0/ now available, thanks again!

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.

None yet

2 participants