Close the file descriptor for add_attachment

[yen3]

If the attachment argument is string, the add_attachment function creates a file descriptor then forget to close it. The patch closes the file descriptor after the post action.

[adehad]

I think you have addressed the problem correctly, but part of me wonders why this function is structured the way it is.

Probably a future pull request though, so don't worry about it now.
I wonder if this function should only accept a string, and the context manager approach be used to open it the intended way (i.e. "rb" mode etc.), this would make the function a lot clearer.

But perhaps I am missing a use case for when the file would already be open ?

[yen3]

Hi, @adehad Thanks for your reply.

https://github.com/pycontribs/jira/pull/957/files#diff-442d14b20d591afd0c3de830d2803e30d4816eedc708f9ad129316aa7e5a4a7cL856

The parameter document mentioned that the function could accept a file descriptor. Maybe I misunderstood something.

[adehad]

Hi, @adehad Thanks for your reply.

https://github.com/pycontribs/jira/pull/957/files#diff-442d14b20d591afd0c3de830d2803e30d4816eedc708f9ad129316aa7e5a4a7cL856

The parameter document mentioned that the function could accept a file descriptor. Maybe I misunderstood something.

Yep you are correct, i was just rambling to myself haha, sorry for the confusion.

[studioj]

it would be nice if you would add a test uploading for example the README file as file descriptor and as string

[yen3]

@studioj Thanks for your review. Remove the unused blocks

[studioj]

@ssbarnea i think this can be merged

[adehad]

@yen3 don't suppose you could run tox -e lint over this. It seems like it is just the linting that is causing the checks to fail.

[yen3]

@adehad @studioj @ssbarnea Thanks for the review. I try to rebase on the latest master branch so I push the change again. Please review it again. Thank you.

[adehad]

@yen3 looks like there is still a minor linting problem, otherwise changes look good to me.

[yen3]

@adehad Thanks for the review. I try to fix it. Please review it again.

[adehad]

added a minor comment on where we do the cast()

[adehad]

oops, there's a cheeky black formatting error somewhere here

[yen3]

@adehad I run black -l 88 client.py to refine the code format.