pylint: clean up import order and groupings

[dcbaker]

This just makes things a bit more idiomatic, and easier to look at and reason about

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 94.11765% with 13 lines in your changes missing coverage. Please review.

Project coverage is 68.24%. Comparing base (eb69fed) to head (a9263a8). Report is 1912 commits behind head on master.

Files	Patch %	Lines
mesonbuild/scripts/regen_checker.py	0.00%	4 Missing :warning:
mesonbuild/backend/xcodebackend.py	0.00%	3 Missing :warning:
mesonbuild/scripts/scanbuild.py	0.00%	3 Missing :warning:
mesonbuild/scripts/delwithsuffix.py	0.00%	2 Missing :warning:
mesonbuild/backend/vs2013backend.py	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10833      +/-   ##
==========================================
- Coverage   68.99%   68.24%   -0.76%     
==========================================
  Files         406      406              
  Lines       88644    88744     +100     
  Branches    19665    19665              
==========================================
- Hits        61164    60565     -599     
- Misses      22881    23603     +722     
+ Partials     4599     4576      -23     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[xclaesse]

LGTM

[eli-schwartz]

TBH I'm not a big fan of multiple-imports.

[eli-schwartz]

wrong-import-position

This is... objecting to whether future.annotations is imported before or after docstrings? Huh. Apparently either one "works". They have to be the first meaningful line of code, which means docstrings are ignored either way.

...

This check actually does seem pretty meaningless as there are exactly two cases where we have "issues", and one of them doesn't really seem to matter (and doesn't contribute for or against code readability! future imports are administrivium and might as well operate on the same level as a PEP 263 "line 2 source code encoding comment"), and the other needs to be out of order and we suppress that with a code smell called suppression comments.

I don't think this check is useful in practice.

[dcbaker]

This also catches some annoying things that I've seen in code reviews like:

import something

def ...

class ...

import something esle

def ...

and I'd like to not have to review that anymore.

But also, the comments simply point out that what we're doing already is a code smell. We're monkey patching the standard lib, and like globals I think it's worthwhile to force the use of those comments because it forces us to really think about whether that's a code solution or not.

[eli-schwartz]

This also catches some annoying things that I've seen in code reviews like:

Have you seen this in code reviews for Meson? Because I really haven't. :eyes: Like I said, I don't think this has really been a problem for Meson, except for it shouting at correct uses, like implementing concepts of monkey patching the stdlib.

That being the case, it feels annoying to add a lint check that basically only exists to force a correct use to comment that it's correct.

But also, the comments simply point out that what we're doing already is a code smell. We're monkey patching the standard lib, and like globals I think it's worthwhile to force the use of those comments because it forces us to really think about whether that's a code solution or not.

I literally cannot comprehend how "the comment about wrong import order is pointing out that this code is a code smell because it monkey patches the stdlib and the stdlib is precious and should not be monkey patched". The two have literally, absolutely, utterly nothing to do with each other, and adding comments about the former isn't even going to maybe a little bit force anyone to think about anything.

The requirement to add comments says exactly one thing: "is having this bad import order necessary to achieve your functional goals? If not, don't do it".

It will force people to say "yep, this bad import order is the only way to monkey patch the stdlib, so the comments are necessary".

It will NOT force people to consider whether monkey patching the stdlib is a good idea. But a lint check that checks for assignments to sys.modules['XXX'] would, however, point out the code smell of monkey patching. If pylint has such a check, I'd be more than happy to enable that instead.

[tristan957]

I only saw one comment to disable the check unless I missed something. Obviously this is a very rare issue for Meson, so if it helps 99% of the time and hurts 1% of the time, that is a gain in my book :). Less things to review the better you all will be.

I think multiple imports on the same line is ugly!

[eli-schwartz]

If 99% of the time is exactly never, then... ?

[tristan957]

This is to protect against people submitting code to Meson with imports out of order. I don't see your point. It is like saying "all my code currently conforms to -Wall -Wextra -pedantic, but I am not gonna add it to my default CFLAGS. It isn't useful because it caught nothing"

[eli-schwartz]

I'm engaging in the additional hot take that no one actually does that, so the warning flag is academic rather than practical.

This wouldn't matter so much except that the codebase has perfectly reasonable code that triggers false positives under our -Wall -Wextra -pedantic analogue, and requires surrounding this reasonable code with:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wall"
#pragma GCC diagnostic ignored "-Wextra"
#pragma GCC diagnostic ignored "-Wpedantic"
// code goes here
#pragma GCC diagnostic pop

#pragma GCC diagnostic ignored is a code smell. # pylint: disable= is a code smell.

[tristan957]

No, it depends on the context whether that is a code smell.

static int __attribute__(printf(1, 2))
func(const char *fmt, ...)
{
  va_list args;

  if (!fmt || *fmt == '\0')
    return EINVAL;

  va_start(args, fmt);
  vfprintf(stdout, fmt, args);
  va_end(args);

  return 0;
}

int
main(void)
{
  int rc;

  rc = func(NULL);
  assert(rc == EINVAL);

  rc = func("");
  assert(rc == EINVAL);

  return 0;
}

This is perfectly valid code on compilers that don't support the printf attribute, but you need #pragma GCCs to exclude the warnings on gcc for example. Only other valid alternative is to not compile those test statements on gcc only, which has no similarity here in the context of this PR.

[eli-schwartz]

Only other valid alternative

You mean, the valid approach. :)