PR: Add an option to show/hide line numbers to History #5363
Conversation
|
Hello @csabella! Thanks for updating the PR.
Comment last updated on October 04, 2017 at 20:26 Hours UTC |
|
I added some tests for history.py, but not complete coverage in case the tests weren't the way you would like them. I tried to follow existing patterns, but I didn't do anything with simulating user activity via qtbot. Please let me know if the tests are OK or if they should have been done differently so that I can make sure to apply the correct style if I write more of them. Thanks! |
spyder/plugins/tests/test_history.py
Outdated
|
|
||
| #============================================================================== | ||
| # Constants | ||
| #============================================================================== |
There was a problem hiding this comment.
Please remove this block comment if it's not needed.
| #============================================================================== | ||
| # Tests | ||
| #============================================================================== | ||
|
|
|
|
||
|
|
||
| @pytest.fixture | ||
| def historylog_with_tab(historylog, mocker, monkeypatch): |
There was a problem hiding this comment.
Please add a docstring to describe what this fixture is about.
| # Tests | ||
| #============================================================================== | ||
|
|
||
| def test_init(historylog): |
There was a problem hiding this comment.
Please add a short docstring to all tests to describe what each one is testing.
There was a problem hiding this comment.
Done. My tests matched to the methods, so I wasn't quite sure what to write. If it's too redundant, I can change it.
|
This is a very good addition @csabella, specially because of all the tests you added! Thanks for taking the time to create them. |
spyder/plugins/history.py
Outdated
| @@ -111,7 +116,8 @@ def __init__(self, parent): | |||
| # Find/replace widget | |||
| self.find_widget = FindReplace(self) | |||
| self.find_widget.hide() | |||
| self.register_widget_shortcuts(self.find_widget) | |||
| if not self.testing: | |||
| self.register_widget_shortcuts(self.find_widget) | |||
There was a problem hiding this comment.
I think It's better to monkeypath this in the test fixture instead of adding test logic to the plugin:
@pytest.fixture
def historylog(qtbot, monkeypatch):
monkeypatch.setattr(
'spyder.plugins.history.HistoryLog.register_widget_shortcuts',
lambda *args: None)
historylog = history.HistoryLog(None, testing=True)
[...]
Please remove the testing arg, and monkeypath the fixture
There was a problem hiding this comment.
Thanks! I had added testing based on test_ipythonconsole, but I agree that the monkeypatch is better.
|
Thanks @csabella, this is going in! |
ccordoba12
changed the title
|
Thank you! |
Fixes #970.