WIP: pylint upgrade

[Cynical-Optimist]

See original merge request on GitLab
In GitLab by [Gitlab user @frazerclews] on Jul 5, 2020, 03:15

Description

//: # Upgrade .pylintrc to pylint 2.5, while also enforcing new rules to make the codebase better

Changes proposed in this merge request:

• Update .pylintrc
• Enforce other issues like LF line endings
• Enforce new style of logging

This merge request, when approved, will close:

---

[Cynical-Optimist]

In GitLab by [Gitlab user @BenjaminSchubert] on Jul 5, 2020, 09:01

Commented on .pylintrc line 131

please add the , back there, it helps reduce the amount of diff when a line needs to be added.

[Cynical-Optimist]

In GitLab by [Gitlab user @BenjaminSchubert] on Jul 5, 2020, 09:01

Commented on .pylintrc line 324

I am not too confident about this one. Do all our dependencies use the newstyle already? Those can't be mixed at a project level.

[Cynical-Optimist]

In GitLab by [Gitlab user @BenjaminSchubert] on Jul 5, 2020, 09:01

Commented on .pylintrc line 402

This should stay disabled, black takes care of it already

[Cynical-Optimist]

In GitLab by [Gitlab user @BenjaminSchubert] on Jul 5, 2020, 09:02

The first commit has a huge number of changes, including weird line-returns. (Cutting words after a '-').

Is that how the vanilla generated pylintrc file look like now?

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 5, 2020, 15:35

Yeah, I just did the usual pylint --generate-rcfile then change the variables so they match the current repo in the first commit. Personally think it's a good idea to do that since it fixes a few grammatical mistakes, and makes porting to newer pylintrc files easier down the line hopefully

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 5, 2020, 15:35

Commented on .pylintrc line 402

That's what I thought, but wasn't too sure if you would want to enforce it in pylint too just in case for consistency, will remove it

[Cynical-Optimist]

In GitLab by [Gitlab user @BenjaminSchubert] on Jul 5, 2020, 15:36

Commented on .pylintrc line 402

Pylint is already quite slow, I think that if we have another tool checking it, it's fine to disable here :) thanks!

[Cynical-Optimist]

In GitLab by [Gitlab user @BenjaminSchubert] on Jul 5, 2020, 15:37

Agreed, I was just surprised at the number of changes. Let's keep it as close to the genuine as possible.

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 5, 2020, 15:40

Commented on .pylintrc line 324

I don't think it causes an issue with dependencies, it's just a style of logging. There are plenty of things in the dependencies I am certain don't follow other rules we enforce and work just fine

[Cynical-Optimist]

In GitLab by [Gitlab user @BenjaminSchubert] on Jul 5, 2020, 15:45

Commented on .pylintrc line 324

In order for logging to work with new style logging, it needs to be configured at the logger level. So if we configure the root logger to use "new style", we'd end up having all the other logger not using new style fail when logging they lines.

We should at least verify that all the logging is correct and doesn't show stack traces, as it would not be an error, logging catches all errors that happen inside it to not crash your app.

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 6, 2020, 11:15

Commented on .pylintrc line 131

changed this line in version 2 of the diff

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 6, 2020, 11:15

Commented on .pylintrc line 402

changed this line in version 2 of the diff

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 6, 2020, 11:15

added 2 commits

• 162fdebe - update current .pylintrc
• 2dba762b - enfore pylint rules, implicit-str-concat, logging-too-many-args

Compare with previous version

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 6, 2020, 11:16

Commented on .pylintrc line 131

Not sure how I feel about that since it goes against the style of the rest .pylintrc, but added the comma back

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 6, 2020, 13:35

added 13 commits

• 2dba762b...e79f4a01 - 10 commits from branch master
• 7a5eca2 - update current .pylintrc
• eaa9052 - enfore pylint rules, implicit-str-concat, logging-too-many-args
• be1da43f - WIP test

Compare with previous version

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 6, 2020, 13:59

added 1 commit

• a983d159 - WIP test

Compare with previous version

[Cynical-Optimist]

In GitLab by [Gitlab user @frazerclews] on Jul 6, 2020, 15:12

added 1 commit

• c0c1144 - WIP test

Compare with previous version

[Cynical-Optimist]

In GitLab by [Gitlab user @cs-shadow] on Jul 14, 2020, 23:19

Thanks for starting this. As a coding style thing, I think it may make sense to separate the pylint upgrade from other changes.

In general, I see value in keeping all the "upgrade" MRs as targeted as possible. That makes reviewing them much simpler, and other changes in the coding style can be debated separately. Merging the upgrade with other changes muddies the water a little bit regarding the scope of the review.

[Cynical-Optimist]

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

mentioned in merge request !2011