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

Fixed TypeError when Port of gvm.connections.SSHConnection is None #321

Merged
merged 19 commits into from Nov 1, 2020

Conversation

Korkmatik
Copy link
Contributor

@Korkmatik Korkmatik commented Oct 31, 2020

What: If None is passed for any value in the constructor of SSHConnection, TLSConnection, UnixSocketConnection or GvmConnection then the default value will be used.

Why: TypeError will be thrown if port is None, even though Port is Optional. Closes #320

How: I've added tests

Checklist:

@Korkmatik Korkmatik requested a review from a team as a code owner October 31, 2020 23:38
@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #321 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #321      +/-   ##
==========================================
+ Coverage   97.02%   97.15%   +0.12%     
==========================================
  Files          18       18              
  Lines        4005     4005              
==========================================
+ Hits         3886     3891       +5     
+ Misses        119      114       -5     
Impacted Files Coverage Δ
gvm/connections.py 57.48% <100.00%> (+2.99%) ⬆️

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 2c3bcd9...5fadbd5. Read the comment docs.

Copy link
Member

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contributions. I really appreciate that!

Passing None and not passing a value for using a default are two different things. When I did add the typing a misunderstood the meaning of Optional. My understanding was it is optional to pass an argument. if not passed the default will be used. But actually Optional[int] means Union[int, None].

So you PR actually implemented the right meaning of Optional but the behavior for the other parameters like hostname is still different. Either passing None should always use the default value or an exception should be raised here instead.

tests/connections/test_ssh_connection.py Outdated Show resolved Hide resolved
@Korkmatik Korkmatik changed the title Fixed TypeError when Port of gvm.connections.SSHConnection is None WIP: Fixed TypeError when Port of gvm.connections.SSHConnection is None Nov 1, 2020
Copy link
Member

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

Just a small thing (assertIsInstance)

tests/connections/test_ssh_connection.py Outdated Show resolved Hide resolved
@Korkmatik Korkmatik changed the title WIP: Fixed TypeError when Port of gvm.connections.SSHConnection is None Fixed TypeError when Port of gvm.connections.SSHConnection is None Nov 1, 2020
@bjoernricks bjoernricks merged commit 7206eea into greenbone:master Nov 1, 2020
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.

TypeError if None is passed as port to the constructor of gvm.connections.SSHConnection
2 participants