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.

Sign up for GitHub

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

Jump to bottom

KeywordBear in 0.11 RC doesn't allow unsetting 'keywords' setting anymore #1689

Closed
underyx opened this issue Apr 29, 2017 · 14 comments · Fixed by #1690
Closed

KeywordBear in 0.11 RC doesn't allow unsetting 'keywords' setting anymore #1689

underyx opened this issue Apr 29, 2017 · 14 comments · Fixed by #1690
Assignees
@meetmangukiya
Labels
area/usability importance/critical needs backport type/bug
Milestone
0.11

Comments

@underyx
Copy link
Member

underyx commented Apr 29, 2017

Try this .coafile:

[keywords]
files = .coafile
language = coafile
bears = KeywordBear
regex_keywords = some_(useful|weird)_regex_pattern
# next line unsets the default 'FIXME' value
keywords =

Instead of yielding 0 results as expected, it will yield 907; one for each occurrence of the empty string.

Pinging @Mixih since it's a release blocker I assume.
@Mixih
Copy link
Member

Mixih commented Apr 29, 2017 •
edited
Loading

Extremely urgent, we should dispatch this immediately.

@underyx
Copy link
Member Author

underyx commented Apr 29, 2017

Also, once this is fixed, please check KeywordBear regex and non-regex performance, it seemed a bit off but it might be cause of this bug, and it was difficult to test it while this issue is still open.

@Mixih
Copy link
Member

Mixih commented Apr 29, 2017

not a regression as it doesn't work with 0.10 and was obviously broken in 0.9. Still extremely urgent

@Mixih Mixih removed the regression label Apr 29, 2017
@Mixih
Copy link
Member

Mixih commented Apr 29, 2017 •
edited
Loading

Triaged. This is an old KeywordBear problem where the empty keywords setting is not handled. Should be fixable by adding a case if the setting is empty(i.e == '').

Marking usability as it is a usecase issue.

@Mixih Mixih added this to the 0.11 milestone Apr 29, 2017
meetmangukiya added a commit to meetmangukiya/coala-bears that referenced this issue Apr 29, 2017
@meetmangukiya
KeywordBear: Do not run if keywords is empty
e6fecb5 
If a setting is used in the coafile and nothing is assigned, then the
value of that key is an empty string ''. Now, since `keywords` is a
`list`, `list('')` returns an empty list: `[]` which leads to
construction of regex `r'()'` which matches every character, and hence
produces false results.

Fixes coala#1689
@meetmangukiya meetmangukiya mentioned this issue Apr 29, 2017
Merged
2 tasks
meetmangukiya added a commit to meetmangukiya/coala-bears that referenced this issue Apr 29, 2017
@meetmangukiya
KeywordBear: Do not run if keywords is empty
1741bdf 
If a setting is used in the coafile and nothing is assigned, then the
value of that key is an empty string ''. Now, since `keywords` is a
`list`, `list('')` returns an empty list: `[]` which leads to
construction of regex `r'()'` which matches every character, and hence
produces false results.

Fixes coala#1689
@meetmangukiya
Copy link
Member

Copy, paste from commit message:

If a setting is used in the coafile and nothing is assigned, then the value of that key is an empty string ''. Now, since keywords is a list, list('') returns an empty list: [] which leads to construction of regex r'()' which matches every character, and hence produces false results.

@Makman2
Copy link
Member

Makman2 commented Apr 29, 2017

Shouldn't we patch the parser instead to consider empty strings to be an empty list?

@meetmangukiya
Copy link
Member

@Makman2 here an empty list is created, so, no. ;)

meetmangukiya added a commit to meetmangukiya/coala-bears that referenced this issue Apr 30, 2017
@meetmangukiya
KeywordBear: Handle empty keywords
e778c52 
If setting keyword is used in coafile and not assigned, then keywords
is an empty list, which leads to construction of regex `r'()'` which
matches all the letters, hence yielding false results.

Fixes coala#1689
@Makman2
Copy link
Member

Makman2 commented Apr 30, 2017

ah nvm have misunderstood your comment above 👍

meetmangukiya added a commit to meetmangukiya/coala-bears that referenced this issue May 2, 2017
@meetmangukiya
KeywordBear: Handle empty keywords
ec1b330 
If setting keyword is used in coafile and not assigned, then keywords
is an empty list, which leads to construction of regex `r'()'` which
matches all the letters, hence yielding false results.

Fixes coala#1689
meetmangukiya added a commit to meetmangukiya/coala-bears that referenced this issue May 2, 2017
@meetmangukiya
KeywordBear: Handle empty keywords
2221295 
If setting keyword is used in coafile and not assigned, then keywords
is an empty list, which leads to construction of regex `r'()'` which
matches all the letters, hence yielding false results.

Fixes coala#1689
@meetmangukiya meetmangukiya mentioned this issue May 2, 2017
Merged
2 tasks
@jayvdb jayvdb reopened this May 2, 2017
@jayvdb
Copy link
Member

jayvdb commented May 2, 2017

Note , the bug fixed in #1690, and backported in #1695 , also occurred in 0.10, and it appears to have also occurred in 0.9 from my limited testing.

@underyx , if you are experiencing a regression in 0.11 RC, which version did this work correctly?

This is tightly related to coala/coala#4116 (unsetting default settings).

@underyx
Copy link
Member Author

underyx commented May 2, 2017 •
edited
Loading

@jayvdb: I found the issue in 0.11.0.rc1. Our last known working version is 0.9.1 (we're using the 0.9 Docker image.)

@underyx
Copy link
Member Author

underyx commented May 2, 2017

Oh nooo, sorry, @jayvdb, I just realized that repo uses the 0.8 image, so last known working version is actually 0.8.1.

@jayvdb
Copy link
Member

jayvdb commented May 2, 2017

Ah, that makes much more sense, as I can reproduce this bug on 0.9.
Now trying 0.8...

@jayvdb
Copy link
Member

jayvdb commented May 2, 2017

yup; confirmed. the regression happened at bfd61fb, and the fix applied in above PRs is correct for that regression.

meetmangukiya added a commit to meetmangukiya/coala-bears that referenced this issue May 2, 2017
@meetmangukiya
KeywordBear: Handle empty keywords
c30c430 
If setting keyword is used in coafile and not assigned, then keywords
is an empty list, which leads to construction of regex `r'()'` which
matches all the letters, hence yielding false results.

Fixes coala#1689
@meetmangukiya meetmangukiya mentioned this issue May 2, 2017
Closed
2 tasks
@Mixih
Copy link
Member

Mixih commented May 2, 2017

closing issue as discussion has been resolved.

@Mixih Mixih closed this as completed May 2, 2017
@meetmangukiya meetmangukiya mentioned this issue May 2, 2017
Merged
2 tasks
gosom pushed a commit to gosom/coala-bears that referenced this issue Jul 15, 2017
@meetmangukiya
KeywordBear: Handle empty keywords
70394c0 
If setting keyword is used in coafile and not assigned, then keywords
is an empty list, which leads to construction of regex `r'()'` which
matches all the letters, hence yielding false results.

Fixes coala#1689
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@meetmangukiya meetmangukiya

Labels
area/usability importance/critical needs backport type/bug
Milestone
0.11
5 participants
@jayvdb @underyx @Mixih @Makman2 @meetmangukiya