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

InvalidLinkBear false positive #758

Closed
sils opened this issue Sep 1, 2016 · 24 comments
Closed

InvalidLinkBear false positive #758

sils opened this issue Sep 1, 2016 · 24 comments

Comments

@sils
Copy link
Member

sils commented Sep 1, 2016

http://www.%s.com/ that's a placeholder and should be ignored.

type/bug

@gitmate-bot
Copy link
Collaborator

Thanks for reporting this issue! A coalaian will look at it soon.

@AbdealiLoKo
Copy link
Contributor

This is why the ignore_regex was created was it not ?
I mean we can't possibly predict this placeholder.

Some examples:
C would have http://www.%s.com/
Python 2.6 and lower can have http://www.{0}.com/
Python 2.7/3 can have http://www.{}.com/ or http://www.{hostname}.com/
BASH can have http://www.$0.com/

@Makman2
Copy link
Member

Makman2 commented Sep 1, 2016

what about special characters in URLs? As far as I remember they use % to indicate special characters
(like www.my-website.com/page1.php?value=Hello%20World)

@AbdealiLoKo
Copy link
Contributor

@sils1297 Do you have something to add in favour ?
Can we close this ?

@sils
Copy link
Member Author

sils commented Sep 2, 2016

I think we should ignore all of those by default because I can't see any of those ever being meant as a real URL

@AbdealiLoKo
Copy link
Contributor

I don't think we should because we'd be maintaining a list for every language which is messy. There are a lot of combinations.

@sils
Copy link
Member Author

sils commented Sep 2, 2016

it's better us maintaining it than each user, fact is that InvalidLinkBear looses users because those template URLs aren't properly recognized by default.

@sils
Copy link
Member Author

sils commented Sep 4, 2016

FWIW coala can deal with all placeholders you mentioned except % already and also has a heuristic to detect e.g. markdown links correctly

@sils sils removed their assignment Sep 4, 2016
@madhur-tandon
Copy link
Member

I would like to be assigned to this issue however I need some details on exactly what I have to implement and change.

@sils
Copy link
Member Author

sils commented Oct 21, 2016

basically: add the testcase to the tests, see it fail, then change the regex to make it pass

@sils
Copy link
Member Author

sils commented Oct 21, 2016

thanks for picking this up! CC @SanketDG if you need further help, he wrote that thing

@madhur-tandon
Copy link
Member

madhur-tandon commented Oct 22, 2016

I think the testcase for the placeholder %s to be ignored is to be added in test_link_ignore_regex(self) function in InvalidLinkBearTest.py right ? Also, how do i actually add it ? Does it have to be added in the list of ignored_URLS ??

ignored_URLs = """
http://sub.example.com
http://sub.example.com/something
""".splitlines()

Also, for seeing it fail, I simply run this InvalidLinkBearTest.py right ?
I am sorry but I am taking some time to get accustomed to this. Since this is my 2nd difficulty/low issue, I really want to learn something out of it.
@SanketDG @Makman2 Help ?

@Makman2
Copy link
Member

Makman2 commented Oct 22, 2016

yeah just add one or two samples to the ignored_URLs list, each of them gets tested in the test file^^

@madhur-tandon
Copy link
Member

madhur-tandon commented Oct 22, 2016

Ok, Just a small lil thing, If I add http://www.%s.com the tests still pass, I guess I am writing it the wrong way here (I have also tried other alternate ways of writing it). How do I use this placeholder within a string ? If possible, Could you give me one example of such a test case that fails the test?

ignored_URLs = """
http://sub.example.com
http://sub.example.com/something
http://www.%s.com ,
""" % (apple).splitlines()

Here since s is a placeholder, it should be replaced with apple
Even this doesn't work :(
The tests still pass
@Makman2

@Makman2
Copy link
Member

Makman2 commented Oct 22, 2016

wait wait wait, you just substitute the ignored_URLs string with apple.splitlines(), whereas apple is not defined and you don't want to substitute. ignored_URLs is a test string where every link on every line is tested against the InvalidLinkBear whether it ignores these links correctly / yields no results on these.

-->

"""
...
http://www.%s.com
""".splitlines()

If your regex works right, then yes the tests shall pass (without substitution), that's what you are trying to achieve^^

@madhur-tandon
Copy link
Member

Oh, Okay, I guess the regex already written before works perfectly fine then. Because http://www.%s.com doesn't show up any error. This means I shall make a PR with one or more such tests only. My PR wont include changes in regex then :)
@Makman2

@Makman2
Copy link
Member

Makman2 commented Oct 22, 2016

sounds good 👍

madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Oct 22, 2016
Add tests to ignore ``%s`` placeholders (without substitution)
in URLs and tests to ignore ``%`` only in URLs

Closes coala#758
@madhur-tandon
Copy link
Member

It turns out I did not know how to run test cases and interpreted wrongly. Will send an updated PR soon.

@Makman2
Copy link
Member

Makman2 commented Oct 22, 2016

no, it shall not be ignored :)

madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Oct 30, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Oct 30, 2016
Add corresponding tests to InvalidLinkBearTest.py
of Modification of regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 16, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc. and add corresponding
tests to InvalidLinkBearTest.py

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 16, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 16, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 16, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 17, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 17, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 29, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 29, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 29, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 29, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 29, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Nov 29, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Dec 1, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Dec 1, 2016
Escape the special meaning of `"` with
a backslash `\` in Regex.

Fixes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Dec 3, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Dec 3, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
madhur-tandon added a commit to madhur-tandon/coala-bears that referenced this issue Dec 4, 2016
Modify regex to include links with %
in between and to ignore placeholders
like %s,%c,%f, etc.

Closes coala#758
@Nosferatul
Copy link
Member

unassigned due to inactivity. If you want to be reassigned, ping!

@Makman2
Copy link
Member

Makman2 commented Feb 2, 2017

Doesn't look like this issue is solved yet^^

@seblat
Copy link
Contributor

seblat commented May 7, 2017

I ran the InvalidLinkBear (coala 0.10.0) on

print('http://www.%s.com' % 'sgoogle') print('http://www.google.com/%s' % 'sgoogle') print('http://www.{}.com'.format('formatgoole'))

and couldnt repoduce the bug.
Has this been fixed already?

@Makman2
Copy link
Member

Makman2 commented May 7, 2017

Indeed looks like it works ;)

@Makman2 Makman2 closed this as completed May 7, 2017
gosom pushed a commit to gosom/coala-bears that referenced this issue Jul 15, 2017
Modify regex to ignore placeholders like
``%s``,``%d``,``%f``, etc. and add corresponding
tests to InvalidLinkBearTest.py

Closes coala#758
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants