Closed amanjiofficial closed 3 years ago
@amanjiofficial Are you sure that this code would even run? Here is the documentation for type hints: https://docs.python.org/3/library/typing.html
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
conpot/protocols/http/web_server.py | 3 | 85.71% | ||
conpot/protocols/ipmi/ipmi_server.py | 6 | 68.22% | ||
conpot/protocols/ftp/ftp_handler.py | 9 | 82.58% | ||
conpot/protocols/ftp/ftp_base_handler.py | 27 | 77.68% | ||
conpot/protocols/http/command_responder.py | 38 | 57.41% | ||
<!-- | Total: | 83 | --> |
Totals | |
---|---|
Change from base Build 1134: | -0.6% |
Covered Lines: | 5391 |
Relevant Lines: | 7567 |
@xandfury CI build is now okay. Please do have a look and suggest adequate changes.
@xandfury CI build is now okay. Please do have a look and suggest adequate changes.
Thank you for your PR. Some recommendations / suggestions:
1) Please use tox locally in order to check if things work before pushing, instead of pushing every single change which triggers Travis :)
2) A commit message should usually reflect what you did since after merging the name and description of the PR will not be incorporated to the main branch. All that will be "left" is a commit that says "changes", which is not exactly helpful ;)
3) If you happen to come up with several commits that basically do the same thing (you don't need to do that many commits if you do your unit testing locally), then please squash them.
Thank you @creolis. I will definitely keep all the points. If the pull request is okay shall I extend type hints to other files as well. Thank you
@amanjiofficial did you had a chance to look into the feedback you have gotten?
@amanjiofficial did you had a chance to look into the feedback you have gotten?
Yes I looked into the feedback and noted the points. If type hints are not implemented yet into the project I would like to take this pull request forward and resolve all the points mentioned in the feedback so that this pull request can be merged. @glaslos
Feel free to go ahead 👍
@amanjiofficial are you having any plans of continuing this?
@glaslos Not plans Right now. I will close the pull request for now.
Fixes #393 I have made changes to
conpot.protocols.bacnet.bacnet_app.py
@xandfury if these changes are okay I would like to extend similar changes to other files as well