.pylintrc: Simplify config, only list overrides

[BuildStream-Migration-Bot]

See original merge request on GitLab
In GitLab by [Gitlab user @cs-shadow] on Aug 3, 2020, 21:36

Rather than having the full .pylintrc, only list the overrides that we
have compared to the default options. This makes it easy to identify
what options we have modified, which is also similar to what we do for
configuring other tools.

At the same time, also remove overrides corresponding to the gi
module. They were added when this repository depended on the ostree
module but that's no longer the case.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @cs-shadow] on Aug 3, 2020, 21:57

added 41 commits

• 795a3dc6...b4a5df28 - 40 commits from branch master
• 4432e6d - .pylintrc: Simplify config, only list overrides

Compare with previous version

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @BenjaminSchubert] on Aug 4, 2020, 07:41

The intent on this MR is widely different compared to another concurrent update to it: !1986

In order to prevent flip-flop between two ways of doing things I think we should discuss this on the ML first.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Aug 4, 2020, 08:13

I believe the original configuration was by [Gitlab user @tlater] and had the specific intent of ensuring that new versions of the linter did not behave drastically different (I think in the current setup, new pylint errors need to be explicitly disabled or fixed in the code in order to upgrade pylint).

Note that pylint support predates our usage of tox, which might in itself obsolete the complicated .pylintrc, but I think this needs to be explicitly confirmed before drastically changing the configuration approach.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @cs-shadow] on Aug 4, 2020, 11:46

Right, that was my understanding as well. Since we pin the pylint versions anyway, I thought that we will notice such issues when trying to update that version to something newer.

I can post a quick message on the list to clarify this.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Aug 4, 2020, 12:04

Right, that was my understanding as well. Since we pin the pylint versions anyway, I thought that we will notice such issues when trying to update that version to something newer.

There is a ton of stuff we don't support from current versions of pylint, even if we are pinning the version, updating it is a chore, I think that the current .pylintrc helps ensure that we can update it easily enough and opt-in to new lint errors more easily and selectively, and we should verify this.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @cs-shadow] on Aug 4, 2020, 21:48

There is a ton of stuff we don't support from current versions of pylint, even if we are pinning the version, updating it is a chore, I think that the current .pylintrc helps ensure that we can update it easily enough and opt-in to new lint errors more easily and selectively, and we should verify this.

I can see where you're coming from but I'm not sure if that's entirely true. This is my understanding of the issue (in a lot of words, sorry about that) :

Having a .pylintrc does not mean that we will not be opting into new pylint checks automatically on new releases. As you can see from the diff, there's no option that expresses "this is the set of checks we want to turn on". We can only disable certain checks, which I'm not proposing to change.

When pylint runs, and we haven't set a config flag, it will still use whatever default it has. So, if we were to do this correctly, we should be regenerating our .pylintrc using the new pylint version each time we update it. We haven't really been doing this so far.

!1986 tries to do this correctly for the next update. I actually proposed this change because I noticed a fair bit of unnecessary diffs for .pylintrc in that patch. That could be avoided if we only list what we're overiding, thereby eliminating the need to regenerate it.

It will be same amount of work in either cases when we are updating pylint. Because, most (if not all) pylint updates don't change default values drastically, but rather add new checks (and fix bugs obviously).

If pylint does make a breaking change, we actually may be better off not having the full .pylintrc. For example, if pylint renames a config option, we won't have to care about renaming it if aren't using it. Which is more likley given that we only use 7 options in total.

I was hoping to be a bit more concise in this comment but we'll have to settle with this :)

I will also be happy to backout this MR if you (or anyone else) have strong feeling one way or the other. Let me know!

[abderrahim]

I think we should merge this. The concern raised that we would need to modify the config in case a new warning is added is incorrect as we already have to make changes whenever we update pylint (see #1494 #1531 #1564).

@cs-shadow do you want to rebase or shall I do it?