PR: Fix QIntValidator to handle '+' sign

[maurerle]

Description of Changes

Somehow Qt QIntValidator allows to enter only a plus sign: '+'

While searching for QIntValidator, I found another occurance in the import dialog of the variable explorer.

• Added unit test(s) covering the changes (if testable)

• Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Supersedes #13333
Fixes #12693

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: Florian Maurer

[dalthviz]

Thanks @maurerle for checking into this! However, testing locally the changes here I think more changes are needed to prevent errors with the go to line dialog. Putting + still makes the Ok button enabled and clicking it causes another error:

Traceback (most recent call last):
  File "e:\acer\documentos\spyder\spyder otros\maurerle\spyder\spyder\plugins\editor\widgets\editor.py", line 862, in go_to_line
    self.get_current_editor().exec_gotolinedialog()
  File "e:\acer\documentos\spyder\spyder otros\maurerle\spyder\spyder\plugins\editor\widgets\codeeditor.py", line 3097, in exec_gotolinedialog
    self.go_to_line(dlg.get_line_number())
  File "e:\acer\documentos\spyder\spyder otros\maurerle\spyder\spyder\plugins\editor\widgets\codeeditor.py", line 3089, in go_to_line
    self.text_helper.goto_line(line, column=start_column,
  File "e:\acer\documentos\spyder\spyder otros\maurerle\spyder\spyder\plugins\editor\utils\editor.py", line 209, in goto_line
    line = min(line, self.line_count())
TypeError: '<' not supported between instances of 'int' and 'NoneType'

I think that case is already covered on the implementation done at #13333

Taking that into account, maybe you could undo the changes done for the go to line functionality here and we could see if we can move #13333 to use 5.x as base and then merge the changes to fix the error on the importwizard dialog you did here?

What do you think?

[maurerle]

I fixed this by checking in the enabled field - which is only two small changes.

But I am also quite emotionless on removing the gotoline part and merging the other PR.
But then I would use the validate_text function in both places and I am not sure if the additional checks are needed after all as they are normally unreachable - as the gotoline dialog cannot be filled with invalid data anymore.
But they don't harm a lot either.

So it is up to you - if you merge the other branch, I will adjust my PR to it :)

[dalthviz]

Thanks for checking the error here @maurerle ! Since you already implemented the handling lets mark this as superseding #13333

I just left a suggestion to clear the line edit too if the input is not valid (the + sign). And a couple of comments to add comments to link the fix with this PR and the issue.

Also, maybe it could be worthy to add a test for this fix to prevent regressions on the validation?

Other than that this LGTM 👍

[maurerle]

Found another bug in the import dialog

[maurerle]

Can you maybe give me a hint on how to add a test for this?

[dalthviz]

Regarding the test maybe a look to the test_recover.py file could be useful. Basically, we could add a new test_gotoline.py file under the same directory the test_recover.py is located, with a test function that checks calling the different methods the dialog has.

[maurerle]

Hi @dalthviz,
I found, that the import dialog does interprete everything as comment if "Comments" is an empty string (which would much rather mean "there are no comments in the file").
Furthermore if Skip Rows is larger than the lines in the import file, an error was thrown.

I hope it is okay to have that second commit in the same PR as it was found while testing, but I can also open another PR for it.

[dalthviz]

@maurerle I think is okay here since it is related with the skip rows value which is something being changed here for the importwizard dialog 👍

[maurerle]

Thanks.
I used:
pip install -e .[test]
and
pytest spyder/plugins/editor/widgets/tests/test_gotoline.py
to test a single_file

Can you give me a hint how I can create the GoToLineDialog?

I currently have:

def test_gotolinedialog_has_cancel_button(codeeditor, qtbot, tmpdir):
    """
    Test that GoToLineDialog has a Cancel button.

    Test that a GoToLineDialog has a button in a dialog button box and that
    this button cancels the dialog window.
    """
    editor = codeeditor
    dialog = GoToLineDialog(editor)
    button = dialog.findChild(QDialogButtonBox).findChild(QPushButton)
    with qtbot.waitSignal(dialog.rejected):
        button.click()

which produces a timeout

[dalthviz]

Maybe something like:

def test_gotolinedialog_has_cancel_button(codeeditor, qtbot, tmpdir):
    """
    Test that GoToLineDialog has a Cancel button.

    Test that a GoToLineDialog has a button in a dialog button box and that
    this button cancels the dialog window.
    """
    editor = codeeditor
    dialog = GoToLineDialog(editor)
    cancel_button = dialog.findChild(QDialogButtonBox).button(QDialogButtonBox.Cancel)
    # Check cancel button is enabled
    assert cancel_button.isEnabled()
    with qtbot.waitSignal(dialog.rejected):
        cancel_button.click()

You could also test more things like the ones related to the changes here with another function like:

def test_gotolinedialog_input(codeeditor, qtbot):
    """
    Regression test for spyder-ide/spyder#12693
    """
    editor = codeeditor
    dialog = GoToLineDialog(editor)
    lineedit = dialog.lineedit
    ok_button = dialog.findChild(QDialogButtonBox).button(QDialogButtonBox.Ok)
    # Check ok button is disabled
    assert not ok_button.isEnabled()
    # Check + sign being cleared and ok button still is disabled
    lineedit.setText("+")
    assert lineedit.text() == None    
    assert not ok_button.isEnabled()
    # Check valid text and ok button enabled
    lineedit.setText("1")
    assert lineedit.text() == "1"    
    assert ok_button.isEnabled()
    with qtbot.waitSignal(dialog.accepted):
        ok_button.click()

[maurerle]

Thank you!
I split the test with valid and invalid into two - as I once learned that you should only test one thing per unit test - on the other hand it creates the dialog twice, which might take longer for running the testing.
Is there any preference in spyder?

I hope it looks good now!
Thanks for introducing me to the unit tests :)

[dalthviz]

Thanks @maurerle for all the work here! Left some suggestions for comments and code formatting but other than that this should be ready for a merge 👍

[maurerle]

Great, thank you! :)

[dalthviz]

Thanks @maurerle !