Skip to content

docs: replace calls to __del__ with calls to close #200

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

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

lafrech
Copy link
Contributor

@lafrech lafrech commented Mar 1, 2021

Related to #190.

Proposed Changes

Use close rather than __del__ in docs, tests and examples.

I could have added a close method to ApiClient for consistency but I didn't.

This SO answer provides a nice explanation of what __del__ is and its limitations and why using it is not a great pattern.

At least, I think the content should be in the close method, and the close method should be called in __del__ rather than the opposite.

Anyway, as said in #190, a context manager would be a nice way of solving the whole thing.

Until then, I figured using close in the docs would be better.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • pytest tests completes successfully
  • Commit messages are in semantic format
  • Sign CLA (if not already signed)

@lafrech lafrech changed the title Replace calls to __del__ with calls to close docs: replace calls to __del__ with calls to close Mar 1, 2021
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #200 (5836c6d) into master (a3e18ee) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   89.88%   89.93%   +0.05%     
==========================================
  Files          26       26              
  Lines        1967     1967              
==========================================
+ Hits         1768     1769       +1     
+ Misses        199      198       -1     
Impacted Files Coverage Δ
influxdb_client/client/write_api.py 97.58% <0.00%> (+0.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3e18ee...5836c6d. Read the comment docs.

@bednar bednar self-requested a review March 2, 2021 08:11
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR 👍

Could you please update a CHANGELOG.md file? Something like:

### Documentation
1. [#200](https://github.com/influxdata/influxdb-client-python/pull/200): Updated docs about `DeleteApi`.

@lafrech
Copy link
Contributor Author

lafrech commented Mar 2, 2021

Done.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your PR 👍

@bednar bednar merged commit eadbf6a into influxdata:master Mar 2, 2021
@bednar bednar added this to the 1.15.0 milestone Mar 2, 2021
@bednar
Copy link
Contributor

bednar commented Mar 2, 2021

@rhajek will take care about #190

@lafrech lafrech deleted the del2close branch March 2, 2021 10:28
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.

2 participants