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

ENH: Make exception thrown by re a bit more informative #4398

Merged
merged 1 commit into from Apr 13, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Apr 11, 2020

Fixes #4393

So now we would get something like

$> datalad search -d /// path:*bu
[ERROR  ] regular expression '(?i)*bu' (original: '*bu') is incorrect: nothing to repeat at position 4 [search.py:_compile_query:934] (ValueError)

instead of

[ERROR  ] nothing to repeat at position 4 [sre_parse.py:_parse:645] (error)

While at it -- completely removed skipped test. It was disabled for a while, and it depends on the external resource (our ///) so could be flaky without our code changes, and takes awhile.

@codecov
Copy link

@codecov codecov bot commented Apr 11, 2020

Codecov Report

Merging #4398 into maint will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4398      +/-   ##
==========================================
+ Coverage   89.63%   89.67%   +0.03%     
==========================================
  Files         275      275              
  Lines       37054    37064      +10     
==========================================
+ Hits        33215    33236      +21     
+ Misses       3839     3828      -11     
Impacted Files Coverage Δ
datalad/metadata/search.py 88.66% <100.00%> (+0.18%) ⬆️
datalad/metadata/tests/test_search.py 95.27% <100.00%> (+0.09%) ⬆️
datalad/support/tests/test_cookies.py 85.71% <0.00%> (-14.29%) ⬇️
datalad/support/tests/test_annexrepo.py 95.38% <0.00%> (-0.31%) ⬇️
datalad/utils.py 84.07% <0.00%> (+0.09%) ⬆️
datalad/downloaders/tests/test_http.py 60.96% <0.00%> (+2.16%) ⬆️
datalad/downloaders/http.py 74.90% <0.00%> (+2.78%) ⬆️

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 14faab4...3675bc1. Read the comment docs.

Copy link
Contributor

@kyleam kyleam left a comment

Some minor comments on the tests. The end result in terms of error message is a nice improvement.

@@ -68,6 +71,7 @@ def test_search_outside1(tdir, newhome):
next(search("bu", dataset=newhome))



Copy link
Contributor

@kyleam kyleam Apr 13, 2020

Choose a reason for hiding this comment

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

Spurious extra space introduced by first commit.

def test_our_metadataset_search(tdir):
# TODO renable when a dataset with new aggregated metadata is
# available at some public location
raise SkipTest
Copy link
Contributor

@kyleam kyleam Apr 13, 2020

Choose a reason for hiding this comment

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

Okay, so this looks to be clean-up of an unrelated test. Based on the comment, it was skipped with the hopes of re-enabling it, but I'm guessing that's a stale expectation for some reason. I can't figure out what that is from the commit message, though:

RF: completely stripping skipped test_our_metadataset_search - will be used in /// itself

In particular, I don't understand what "will be used in /// itself" means.

Copy link
Member Author

@yarikoptic yarikoptic Apr 13, 2020

Choose a reason for hiding this comment

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

I am adding such tests to http://datasets.datalad.org/ repository itself, and will add a run on it to datalad-extensions. We could even add it to our workflows in this repository.
Indeed it is unrelated. will do a dance to submit it into a separate PR

with assert_raises(ValueError) as cme:
ds.search('*wrong')
assert_re_in(
'regular expression .\(\?i\)\*wrong. \(original: .\*wrong.\) is incorrect:',
Copy link
Contributor

@kyleam kyleam Apr 13, 2020

Choose a reason for hiding this comment

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

You're missing an 'r' prefix here. Since Python 3.6, these will give a deprecation warning:

W605 invalid escape sequence '\('
W605 invalid escape sequence '\?'
W605 invalid escape sequence '\)'
W605 invalid escape sequence '\*'
W605 invalid escape sequence '\('
W605 invalid escape sequence '\*'
W605 invalid escape sequence '\)'

The .s in the regular expression made me pause. They match '. Given that it's our code that's adding these quotes, why not just match them explicitly, as we're already doing for the rest of the message?

Copy link
Member Author

@yarikoptic yarikoptic Apr 13, 2020

Choose a reason for hiding this comment

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

couldn't they also be "? At least in python2 repr could have produce one or another IIRC.
So, it was my lazy way to catch both, relaxing a bit too much. Will make it strict

Copy link
Member Author

@yarikoptic yarikoptic Apr 13, 2020

Choose a reason for hiding this comment

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

doh -- I do not use repr ;) so can indeed just place '

@yarikoptic yarikoptic added the merge-if-ok label Apr 13, 2020
It will now include actual regular expression which lead to the failure,
since we could have multiple specified
kyleam
kyleam approved these changes Apr 13, 2020
@kyleam kyleam merged commit aa2da5c into datalad:maint Apr 13, 2020
11 checks passed
@yarikoptic yarikoptic deleted the enh-wrong-re branch Apr 13, 2020
@yarikoptic yarikoptic added this to the 0.12.6 milestone Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants