[PATCH] D150782: [NFC][Py Reformat] Reformat python files in mlir subdir

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

In D150782#4361096 <https://reviews.llvm.org/D150782#4361096>, @ftynse wrote:

> MLIR has been using YAPF with Google style for a while for Python code. Running that results in a relatively clean diff (https://reviews.llvm.org/D151112) compared to near-complete reformatting here. I wonder if it would be preferable to stick to the existing style here. If not, we should also remove the format files like this one: https://github.com/llvm/llvm-project/blob/main/mlir/python/.style.yapf. I'll post on discourse.

I am more concerned about divergence within the project than a one time diff here (it'll be easier to benefit from a pre-merge linter, have a unified doc and workflow for contributors, etc), and it is true that historically the LLVM project has been discouraging mass-fixes like this one, but MLIR has been more aggressive with this kind of mass clang-tidy / clang-format fixes as well.

Let's be more practical: looking at the diff, do you see regressions in readability / maintainability with the new format @ftynse ? 
I see mostly neutral to positive changes.

There a things were neither the old nor the new one are optimal to me, but neither are obviously worse than the other, for example:

  if (
      isinstance(attribute, types.FunctionType)
      and attribute_name.startswith(function_prefix)
  ):
      module_functions.append(attribute)

becomes:

  if isinstance(attribute, types.FunctionType) and attribute_name.startswith(
      function_prefix
  ):
      module_functions.append(attribute)

I would likely prefer to split on the `and` somehow:

  if (isinstance(attribute, types.FunctionType) and
      attribute_name.startswith(function_prefix)):
      module_functions.append(attribute)

Anyway, seems like the migration to black in LLVM is already pretty far, unless we find problems with it in the diff, I think we should just land this!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150782/new/

https://reviews.llvm.org/D150782