[cfg] Upgrade to pylint 3.2.4 #4279

bruntib · 2024-06-27T15:52:21Z

No description provided.

.pylintrc

cservakt · 2024-07-02T11:18:41Z

.pylintrc

-# Regular expression matching correct method names
-method-rgx=[a-z_][a-z0-9_]{2,30}$
+# Maximum number of characters on a single line.
+max-line-length=100


Previously, the maximum number of characters in a line was 80. Is it an automatically generated new value of max-line-length? Shouldn't have we sticked to previous number?

Good point, thanks! pycodestyle is also checking this. By the way, pylint looks quite extensive in format checking. Would it be worth getting rid of pycodestyle? Would we lose anything with it? We could compare these two tools sometime in the future.

Yes, I think it is redundant. We should check these parameters by one tool.

cservakt · 2024-07-02T11:19:02Z

.pylintrc

-# Naming hint for method names
-method-name-hint=[a-z_][a-z0-9_]{2,30}$
+# Maximum number of lines in a module.
+max-module-lines=1000


The max-module-lines limit was 2000. Should we change it?

Maybe, we could, but I turned off max line checking in the disable section anyway. report_server.py has more than 4300 lines :)

cservakt · 2024-07-02T11:19:17Z

.pylintrc

+        duplicate-code,  # TODO: Could be eliminated
+        cyclic-import,  # TODO: Could be fixed
+        subprocess-popen-preexec-fn,  # TODO: Could be fixed
+        broad-exception-caught,  # TODO: could be valid
+        no-member,  # TODO: Why is this emitted for multiprocess module?
+        consider-using-with,  # TODO: Mainly for Popen, could be valid
+        protected-access,  # TODO: Used in test code, but not elegant
+        global-statement,
+        consider-using-get,  # Unnecessary style checker
+        attribute-defined-outside-init  # Too many reports from test codes


The cyclic-import and the no-member checkers used to be enabled explicitly.

I don't understand, why they didn't report earlier, then. Currently they give reports. I added to TODO for cyclic import, that could be solved sometime. It's interesting, though, that python interpreter doesn't bother about it.

I don't understand either. OK, we're going to figure it out and solve the cyclic import problem.

cservakt · 2024-07-02T11:19:43Z

.pylintrc

+#output-format=
+
+# Tells whether to display a full report or only the messages.
+reports=no


We used to set output-format=text and reports=yes. I am not sure it is necessary to change them. Maybe the default value of the output-format is text.

It's interesting, because some reports are generated even without this option. But you're right. I'll check if there are some local settings in Makefiles for pylint invocations. Now I can see some --ignore and some other configurations in Makefiles, but you're right, every check should use the same configuration. I'll unify pylint calls in makefiles.

Thank you for checking it!

cservakt · 2024-07-02T11:34:47Z

analyzer/codechecker_analyzer/analyzers/analyzer_types.py

@@ -164,7 +164,8 @@ def check_supported_analyzers(analyzers):
            failed_analyzers.add((analyzer_name,
                                  "Failed to detect analyzer binary!"))
            continue
-        elif not os.path.isabs(analyzer_bin):
+
+        if not os.path.isabs(analyzer_bin):
            # If the analyzer is not in an absolute path, try to find it...


Why can't we use elif here?

We can. The checker says that if a continue/break/return/raise/etc. command skips the rest of the block, then it's not necessary to put that block under an "else". I would say, this is a question of style. If we don't agree with this convention, then I can turn off this checker.

cservakt · 2024-07-03T10:37:31Z

web/tests/functional/report_viewer_api/test_get_run_results.py

@@ -87,7 +87,7 @@ def __check_bug_path_order(self, run_results, order):
    def test_get_run_results_no_filter(self):
        """ Get all the run results without any filtering. """
        runid = self._runid
-        logging.debug('Get all run results from the db for runid: ' +
+        logging.debug('Get all run results from the db for runid: %s',


cservakt · 2024-07-03T10:38:43Z

web/tests/functional/report_viewer_api/test_report_filter.py

I prefer to use f-string instead of %s format.

cservakt · 2024-07-03T10:39:26Z

web/tests/functional/skip/test_skip.py

@@ -189,7 +189,7 @@ def test_skip(self):
        """ There should be no results from the skipped file. """

        runid = self._runid
-        logging.debug('Get all run results from the db for runid: ' +
+        logging.debug('Get all run results from the db for runid: %s',


I prefer to use f-string instead of %s format.

cservakt · 2024-07-03T10:40:12Z

web/tests/functional/suppress/test_suppress_generation.py

@@ -237,7 +237,7 @@ def test_suppress_comment_in_db(self):
        Exported source suppress comment stored as a review status in the db.
        """
        runid = self._runid
-        logging.debug("Get all run results from the db for runid: " +
+        logging.debug("Get all run results from the db for runid: %s",


cservakt · 2024-07-03T10:40:26Z

web/tests/functional/suppress/test_suppress_generation.py

+        for run_result in run_results:
+            logging.debug(run_result)
        self.assertIsNotNone(run_results)
        self.assertNotEqual(len(run_results), 0)

-        for bug_hash in hash_to_suppress_msgs:
-            logging.debug("tesing for bug hash " + bug_hash)
-            expected_data = hash_to_suppress_msgs[bug_hash]
+        for bug_hash, expected_data in hash_to_suppress_msgs.items():
+            logging.debug("tesing for bug hash %s", bug_hash)


bruntib · 2024-07-03T13:41:28Z

Thanks for the review, it's a great work at this PR :)

So, would you think, that we should rather allow the "elif" style? Is there any style checker that anyone doesn't agree with? The other common note was about f-strings, but I would say that for the logging library it's reasonable not to use them.

I would wait a little before I go through this change again, so it can be done only once. If no other suggestions arrive, I'll undo "elif" style.

whisperity · 2024-07-04T08:46:22Z

So, would you think, that we should rather allow the "elif" style?

The general consensus (at least from what I understood from the community posts) is that you should adhere to "no elif after continue/break/return" if the if branch is some sort of an "edge case", where we are basically "early returning".

if not foo:
  log("error")
  return

good_things()

If the two (or all) branches of the if is structurally the "same level", such as when we are translating from an enum to a string or the other way, it's better to have them demarcated by elifs. Especially because Python does not support switch cases. ☹️

if foo == enum.A:
  return "A"
elif foo == enum.B:
  return "B"
# ...

bruntib added dev env ⛑️ Development environment config ⚙️ dependencies 📦 Pull requests that update a dependency file labels Jun 27, 2024

bruntib added this to the release 6.24.1 milestone Jun 27, 2024

bruntib requested a review from cservakt June 27, 2024 15:52

bruntib requested a review from vodorok as a code owner June 27, 2024 15:52

bruntib force-pushed the pylint_upgrade branch 4 times, most recently from fccd454 to 9525937 Compare July 1, 2024 11:20

whisperity requested changes Jul 2, 2024

View reviewed changes

.pylintrc Outdated Show resolved Hide resolved

bruntib force-pushed the pylint_upgrade branch from 9525937 to daade11 Compare July 3, 2024 08:08

cservakt reviewed Jul 3, 2024

View reviewed changes

bruntib force-pushed the pylint_upgrade branch from daade11 to 3abb0e9 Compare July 5, 2024 12:33

[cfg] Upgrade to pylint 3.2.4

5df2d07

bruntib force-pushed the pylint_upgrade branch from 3abb0e9 to 5df2d07 Compare July 5, 2024 13:23

cservakt self-requested a review July 8, 2024 11:57

cservakt approved these changes Jul 8, 2024

View reviewed changes

whisperity approved these changes Jul 8, 2024

View reviewed changes

bruntib merged commit 981365c into Ericsson:master Jul 8, 2024
7 of 8 checks passed

bruntib deleted the pylint_upgrade branch July 8, 2024 14:58

bruntib mentioned this pull request Aug 26, 2024

fix pylint unused-argument warnings #2081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cfg] Upgrade to pylint 3.2.4 #4279

[cfg] Upgrade to pylint 3.2.4 #4279

bruntib commented Jun 27, 2024

cservakt Jul 2, 2024

bruntib Jul 3, 2024

cservakt Jul 5, 2024

cservakt Jul 2, 2024

bruntib Jul 3, 2024

cservakt Jul 2, 2024

bruntib Jul 3, 2024

cservakt Jul 5, 2024

cservakt Jul 2, 2024

bruntib Jul 3, 2024

cservakt Jul 5, 2024

cservakt Jul 2, 2024

bruntib Jul 3, 2024

cservakt Jul 3, 2024

cservakt Jul 3, 2024

cservakt Jul 3, 2024

cservakt Jul 3, 2024

cservakt Jul 3, 2024

bruntib commented Jul 3, 2024

whisperity commented Jul 4, 2024

[cfg] Upgrade to pylint 3.2.4 #4279

[cfg] Upgrade to pylint 3.2.4 #4279

Conversation

bruntib commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruntib commented Jul 3, 2024

whisperity commented Jul 4, 2024