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

Fix abbreviator if name has parameters with dots #147

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

StefRe
Copy link
Contributor

@StefRe StefRe commented Apr 15, 2020

The name of a test is the dotted test function name possibly followed
by a list of parameters in brackets. These parameters can contain dots
as for example in test_unittestwidget_process_finished_updates_status_label
from test_unittestgui.py where the label of the second parameter ends
with a dot, which led to the wrong abbreviation 's.w.t.test_u.t.]'. This
is fixed by not taking into account the parameter list when abbreviating.

Current:
falsch
Fixed:
richtig

The name of a test is the dotted test function name possibly followed
by a list of parameters in brackets. These parameters can contain dots
as for example in test_unittestwidget_process_finished_updates_status_label
from test_unittestgui.py where the label of the second parameter ends
with a dot, which led to the wrong abbreviation 's.w.t.test_u.t.]'. This
is fixed by not taking into account the parameter list when abbreviating.
@jitseniesen
Copy link
Member

I noticed this too but did not find the time / could not be bothered to fix it, so I'm glad to see this PR.

@jitseniesen
Copy link
Member

The code looks good. I can't think of another reason why a test name should have a [ character in it.

Just one request: Could you please update the doc string of the Abbreviator class to note the special handling of the brackets?

class Abbreviator:
"""
Abbreviates names so that abbreviation identifies name uniquely.
First, all names are split in components separated by full stop (like
module names in Python). Every component is abbreviated by the smallest
prefix not shared by other names in the same directory, except for the
last component which is not changed.

Just for your information, I am planning on doing a minor release of a plugin in a few days, mainly to get the TextEditor fix out but it is a good opportunity to also include your other fixes, including this one. Do let me know if you are working on stuff that you think should be included as well.

to explain the procedure introduced in commit #
35494ab.
@StefRe
Copy link
Contributor Author

StefRe commented Apr 16, 2020

I updated the docstring as requested (not being a native speaker of English I hope it's more or less understandable).

Currently I'm working on #127 - I found the error but, in order to cover all possible cases, the fix may require some more changes or decisions about how to report test results when there are errors in the setup or teardown functions. So I guess you can do the release without it and we can discuss this issue later.

@jitseniesen
Copy link
Member

Excellent work. I fixed two typos, one of which was due to myself. Don't worry about your English. Almost none of the people working on Spyder are native speakers of English, myself included, and it looks like your English is better than most. Vielen Dank!

@jitseniesen jitseniesen merged commit bc03aee into spyder-ide:master Apr 17, 2020
@StefRe StefRe deleted the fix/abbreviator branch April 17, 2020 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants