These are chat archives for coala/coala-bears

29th
Oct 2018
Rishabh Garg
@rishabhgarg25699
Oct 29 2018 05:11
@jayvdb I found an issue i.e coala/coala-bears#2465 and IMO due to this issue my pr i.e coala/coala-bears#2702 is not working which you close it . I am saying the same thing which this issue is saying when i solving my issue.
John Vandenberg
@jayvdb
Oct 29 2018 05:24
@rishabhgarg25699 , firstly and most importantly (ill follow up with a more positive second message), do not act indignant about your PR. Your tests where horrifically wrong. You were informed how to test your code by multiple reviewers , very clearly, and the test logic that you push in your PR, many times, did not follow the instructions given.
you should have found that issue much earlier. A simple search on existing bugs would have found it. that is analysis. you do that before coding.
John Vandenberg
@jayvdb
Oct 29 2018 05:32
now if you read that issue you found very carefully, and not with the slanted mindset of trying to justify yourself, you will better understand when , and when not, the max line length of that bear will or will not be effective.
you are correct that it is relevant, and I am happy to see that you are still curious about the problem you were trying to solve.
maybe ... if you can properly understand the problem, we'll re-assign it back to you. but before that, you should try some simpler tasks, so you can get back onto a positive direction.
Rishabh Garg
@rishabhgarg25699
Oct 29 2018 05:35
But now i work in coala bears repo or moban repo on gitlab
That you suggested me
John Vandenberg
@jayvdb
Oct 29 2018 05:36
but currently it is assigned to @bkhanale , and maybe they will solve it, and if they do it will be in part because they can see what didnt work, due to your closed PR.
@rishabhgarg25699 , find an issue that you know how to solve. without any help.
that is how you will get your positive direction back.
Rishabh Garg
@rishabhgarg25699
Oct 29 2018 05:37
Ok thanks for the advise
Abhinav Kaushlya
@abhishalya
Oct 29 2018 05:40
Yeah, when it was assigned to @bkhanale he told me about that issue. I think @rishabhgarg25699 you could've done the same since there was no point in spending >25 days on that. I guess now that issue is blocked by another issue, so I think that needs to be solved first.
John Vandenberg
@jayvdb
Oct 29 2018 05:41
it is not blocked by the other issue.
but the other issue is relevant to understanding how to do this issue correctly.
Abhinav Kaushlya
@abhishalya
Oct 29 2018 05:42
Yeah, but I think the tests would fail, won't they?
Rishabh Garg
@rishabhgarg25699
Oct 29 2018 05:42
Yeah imo also test are failing due to that issur
Abhinav Kaushlya
@abhishalya
Oct 29 2018 05:43
In your case, you have wrong tests @rishabhgarg25699
Rishabh Garg
@rishabhgarg25699
Oct 29 2018 05:45
Abhinav Kaushlya
@abhishalya
Oct 29 2018 05:48
@rishabhgarg25699 That is correct. You did it wrong
John Vandenberg
@jayvdb
Oct 29 2018 05:53
you can not just copy test code from another bear
anyways, enough talking about your previous closed PR. If you would like a independent review of it, go to https://coala.io/coc . otherwise leave it alone and move on
Bhushan Khanale
@bkhanale
Oct 29 2018 06:20
The issue does not affect directly but is also means that the tests are not very useful. Since, when I'm passing max_line_length: 0 or anything it still passes.
John Vandenberg
@jayvdb
Oct 29 2018 07:35
cripes. read coala/coala-bears#2465 carefully. all of it
top to bottom
make sure you understand it
Bhushan Khanale
@bkhanale
Oct 29 2018 07:54
@jayvdb I understood it very well, unfortunately when I ran PEP8NotebookBear on a .ipynb with a long code line, it didn't throw an error. So https://github.com/coala/coala-bears/issues/2465#issuecomment-393624856 didn't really worked for me.
Bhushan Khanale
@bkhanale
Oct 29 2018 08:02
Same with the PEP8Bear, an issue was already created here: coala/coala-bears#1523
John Vandenberg
@jayvdb
Oct 29 2018 08:32
ok ... ill look at again .. maybe I am biased because I was looking at test code which wasnt suitable
Bhushan Khanale
@bkhanale
Oct 29 2018 09:28

@jayvdb Just a small issue. Since I have to test the max_line_length to be working, I have to create a file with more than 80 chars. Since to avoid the PEP8 error, I constructed the file like this: https://github.com/coala/coala-bears/pull/2742/files#diff-13e02d940392c937cb4ca9f2ef8cc925R91

But the tests fail here, I think its not able to verify the file correctly.

Any other option here? ^
Abhinav Kaushlya
@abhishalya
Oct 29 2018 09:59
@bkhanale Interesting, the code changes looks good to me except the file itself.
@li-boxuan can you review coala/coala-bears#2742
John Vandenberg
@jayvdb
Oct 29 2018 12:16
@bkhanale while you wait, you can look at solving the upstream problem
Bhushan Khanale
@bkhanale
Oct 29 2018 12:20
@jayvdb I would do that, but currently I'm facing different issue. The tests should pass ideally, but they are not. Seems like an issue with JSON file validation.
The thing is if I use the source string in a single line without breaking it up, ignoring the 80 char limit, the tests pass. But as soon as I use multi line string in JSON, it is throwing me an error. Although the JSON is valid.
John Vandenberg
@jayvdb
Oct 29 2018 12:34
does JSON support multiline string ?
Bhushan Khanale
@bkhanale
Oct 29 2018 12:38
I think it does not. But the changes worked fine with my notebook
I'll have to ignore PEP8Bear then for that file
Bhushan Khanale
@bkhanale
Oct 29 2018 13:32
@jayvdb The build is passing. Requesting reviews now :)
John Vandenberg
@jayvdb
Oct 29 2018 14:07
merged
@bkhanale want to keep going down that rabbit hole ... coala/coala-bears#2743 ?
Bhushan Khanale
@bkhanale
Oct 29 2018 14:09
@jayvdb Yep, sure :)
John Vandenberg
@jayvdb
Oct 29 2018 14:28
assigned, and can you also figure out which issue this is : https://github.com/coala/coala-bears/pull/2707/files
if none, create a new issue. note on the PR. and ping the contributor here, etc.
@sladyn98 , how you going with coala/coala-bears#2688 - it is soooo close
Bhushan Khanale
@bkhanale
Oct 29 2018 15:36

@Ishaan28malik Your PR coala/coala-bears#2707 has no use since the issue has already been cleared as you can see https://github.com/coala/coala-bears/blob/master/bears/python/PEP8NotebookBear.py#L12

So I guess you can close your PR now. @jayvdb