These are chat archives for coala/coala-bears

2nd
Feb 2019
John Vandenberg
@jayvdb
Feb 02 03:17
@frextrite done
Naveen Naidu
@Naveenaidu
Feb 02 05:49
@frextrite @sladyn98 @jayvdb Please have a look at coala/coala-bears#2848 . Commit message updated and I explained my reasons for the approach I used there.
Amol Grover
@frextrite
Feb 02 09:10
Travis will still fail until W605 is fixed and merged into the branch.
@jayvdb I've just updated gometalinter in this PR. We're still using old golang v1.7.4. Should I update that to the latest stable(v1.11)? I've done preliminary testing and the latest version seems to be working with no issues.
Naveen Naidu
@Naveenaidu
Feb 02 09:15
Won't upgrading the golang remove the problem with gometalinter?
I mean, why not directly upgrade golang to the latest version, that way gometalinter will not fail in the first place.
Amol Grover
@frextrite
Feb 02 09:18
The way gometalinter linters are currently installed(gometalinter --install) is now deprecated and so is the way gometalinter is currently downloaded and installed, hence both of these need to be fixed and optionally golang needs to be updated
Naveen Naidu
@Naveenaidu
Feb 02 09:21
Okay! But doesn't the line curl -L https://git.io/vp6lP | sh install the lastest stable release.
Why do you have to use the method of disabling all and enabling only required bears?
iirc the problem we had was that due the gometalinter dropping the support for old go versions, some of the functions used the linters of gometalinter became outdated and our builds failed. But if we just upgrade the go version then we no longer would face the Invalid data type functions thrown by the linter. (New data types have been added in the newer versions of go, which are used by the linters of gometalinter as a result of which, our current go version faces a problem)
Enabling only our required bear would only be a quick fix. But as stated in the comment by John, doing the above method we cannot maintain 100% test coverage
Amol Grover
@frextrite
Feb 02 09:34

Okay! But doesn't the line curl -L https://git.io/vp6lP | sh install the lastest stable release.

Correct

Why do you have to use the method of disabling all and enabling only required bears?

bears linters I hope? Take a look at the FAQs https://github.com/alecthomas/gometalinter#whats-the-best-way-to-use-gometalinter-in-ci point 2

Naveen Naidu
@Naveenaidu
Feb 02 09:37
Ah! yes linters :P - Checking the link out
Amol Grover
@frextrite
Feb 02 09:37

Quoting from https://github.com/alecthomas/gometalinter/releases/tag/v2.0.0

Another major change is that from now on we will be releasing binary packages. gometalinter's management of linter installation will soon be deprecated, and eventually removed.

Which basically means gometalinter --install will be(is?) deprecated.
Naveen Naidu
@Naveenaidu
Feb 02 09:42
I completely agree with your decision of removing --install method. But It's just the disabling and enabling of linters is what worries me. This would mean that even when a new linter is added in gometalinter, the CI won't know about it, and we would have to explicitly add the linters to it.
That also means, we would have to keep track of activities of gometalinter.
The 2nd point in the page you referred, does say that a new linter might be a problem. But don't we want all the linters of gometalinter to run on our files?
Or is my understanding wrong about this?
Naveen Naidu
@Naveenaidu
Feb 02 09:51
And also if I am not wrong, curl -L https://git.io/vp6lP | sh install installs all the linters it has. It is not necessary for us to disable all the linters and then enabling it one one.
John Vandenberg
@jayvdb
Feb 02 10:01

Travis will still fail until W605 is fixed and merged into the branch.

We will stack several PRs on top of each other to get all the fixes lined up, and CI fixed, then merge them all at once

I completely agree with your decision of removing --install method. But It's just the disabling and enabling of linters is what worries me. This would mean that even when a new linter is added in gometalinter, the CI won't know about it, and we would have to explicitly add the linters to it.

We dont have any other option for old go , and we dont use all of their go linters - coala only uses a subset of them. maybe this is what @utkarsh2102 was meaning. we dont care if they add new linters; we need a bear which uses them. We still do not have a generic GoMetaLinterBear which might uses them all.

Naveen Naidu
@Naveenaidu
Feb 02 10:06
But what would happen if one of the linter that out bear use, drops supports for the go version we are using?

We dont have any other option for old go

I'm sorry but I don't completely understand the above statement.

John Vandenberg
@jayvdb
Feb 02 11:57
If a go linter stops working, our ci fails again
Amol Grover
@frextrite
Feb 02 12:49
Which version of golang should we use in CI?
1.10.8(old stable)? 1.11.5(latest stable)? or the latest version available at the time of build?
John Vandenberg
@jayvdb
Feb 02 12:54
Doesnt matter. Any version that runs the linters
Amol Grover
@frextrite
Feb 02 13:02
Hardcoded v1.11.5 so that future versions don't break CI unexpectedly.
Naveen Naidu
@Naveenaidu
Feb 02 13:44
Awesomee!! Sentinel goes green again.
The only problem that we have now is with TextLintBearTest.py a typo in the test message. Should we fix it too? Logs
Amol Grover
@frextrite
Feb 02 13:53
Creating an issue for it
Sladyn
@sladyn98
Feb 02 14:24
Yeah just check for duplicate issues
Sladyn
@sladyn98
Feb 02 14:45
We have an assertion error the text should actually be PSF" is unexpanded acronym. What does "PSF" stand for?', instead of stands for
If you want I could submit a PR
Naveen Naidu
@Naveenaidu
Feb 02 15:15
@sladyn98 I'm testing the changes on my fork with all the changes we have made until now along with the assertion one. That would let us know if these are the only errors. If they work out, I'll let you know you can make a PR then :)
Naveen Naidu
@Naveenaidu
Feb 02 15:29
@jayvdb @sladyn98 @frextrite Finally the build is green :tada: , Tested it on my fork by rebase onto all the commits.

If you want I could submit a PR

Please go ahead. I think that would be the final PR that is needed to pass the builds

Sladyn
@sladyn98
Feb 02 15:35
Yesssssssss @Naveenaidu thats is what we all wanted .Hopefullly corobo's will build as well
Naveen Naidu
@Naveenaidu
Feb 02 15:38
Yeah :)
Amol Grover
@frextrite
Feb 02 17:26
🎉🎉
Kindly review, hopefully the last of the changes in our effort to make the CI green
Naveen Naidu
@Naveenaidu
Feb 02 18:51
@sladyn98 you missed another typo error
It's in the function test_bad_ng_words_acronym_list_item
Sladyn
@sladyn98
Feb 02 19:18
fixed