Skip to content

json_dump: draft of reconnect feature#217

Merged
jaroslavpesek merged 3 commits intomasterfrom
json_dump_reconnecting
Apr 4, 2023
Merged

json_dump: draft of reconnect feature#217
jaroslavpesek merged 3 commits intomasterfrom
json_dump_reconnecting

Conversation

@cejkato2
Copy link
Copy Markdown
Contributor

No description provided.

@cejkato2 cejkato2 marked this pull request as draft October 16, 2022 19:52
@hejcman
Copy link
Copy Markdown

hejcman commented Nov 21, 2022

Code review comments:

  • Avoid using optparse, it is deprecated since Python 3.2. Use argparse instead.
  • It is unnecessary to do manual checks for argument exclusivity, argparse has built in support for mutually exclusive groups.
  • Prefer placing as much code as possible into functions. If the file is imported, any code writen in the main body of the file is executed, which could for example overwrite your global variables.
  • Avoid unecessary commands such as del (e) when e goes out of scope anyway and pass.
  • Remove unused imports.
  • Where possible, write typehints for function or important variables.

@cejkato2 cejkato2 marked this pull request as ready for review November 23, 2022 17:13
Comment thread json_dump/json_dump.py Outdated
Comment thread json_dump/json_dump.py Outdated
Incorporated a fix from PR220 and suggestions by jaroslavpesek, and made a few other improvements.
@jaroslavpesek jaroslavpesek merged commit 534e06c into master Apr 4, 2023
@cejkato2 cejkato2 deleted the json_dump_reconnecting branch April 4, 2023 07:43
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.

4 participants