-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Added filepointer entries #885
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
Conversation
Hmmm the pypy took too long.. Can't see my changes should do that ? |
1 similar comment
3 similar comments
3 similar comments
except ImportError: # pragma: no cover | ||
# noinspection PyUnresolvedReferences | ||
from UserDict import DictMixin as MutableMapping | ||
from UserDict import DictMixin as MutableMapping # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this "code coverage ignore" is worth it - just take the code coverage report with a grain of salt, understand that it's just a tool, and 100% code coverage doesn't actually guarantee everything is tested anyway.
(Side note: all the coveralls PR comments are annoying. I've seen other projects that have coveralls instead put the score as an extra commit status right next to the CI result, which I prefer.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth it when I try to develop and uses hotkeys to jump to next covered part :) But they can be removed if you wishes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I have no authority here, just trying to offer my opinion (to perhaps help refine things or move them along), without being annoying :)
2 similar comments
1 similar comment
2 similar comments
Refactored a lot of tests Removed the need for a real file Made it more generic Added some test Fixed for 26 Fixed wrapper for raises
5 similar comments
@bitprophet will you take a look? I should pass for all version. I rewrote some tests, to be more consistent and remove the need to actually read or write a file. |
self._system_host_keys.load(filename) | ||
|
||
with open(filename, 'r') as fp: | ||
self._system_host_keys.load_fp(fp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since HostKeys.load()
does exactly this ("with open() ...") you might as well let it, rather than duplicate that (admittedly tiny amount of) code here. Same argument applies to load_host_keys()
below.
return e.key | ||
for entry in self._entries: | ||
if entry.key.get_name() == key: | ||
return entry.key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These diff sections just renaming e
to entry
are not really needed in this PR IMHO.
Allowing io.StringIO to be used.
Also cleaned up the save, so HostKey's save method is used, like the load was used.
I would also comment on the load before save... If I delete a key and try to save, wouldn't that load the key back and thereby not deleting it ?