[PATCH] D111042: flang-omp-report summarising script

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Hey @josh.mottley.arm , thanks for taking over! I like how you have refactoring this and split into functions - makes a lot of sense! Also, great job documenting them!

Currently this script is somewhere between a "Python Script" and a "Python Module". Take a look idiomatic usage <https://docs.python.org/3/library/__main__.html#idiomatic-usage> for python scripts. Basically, in a script everything should be inside functions. Also, things inside the main block (i.e. `if __name__ == "__main__"`) should be kept to the bare minimum. Could you update accordingly? This shouldn't be too much work.



================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:58
+
+yaml = YAML()
+
----------------
When `ruamel.yaml` is not present, the script will still run all the way down here and fail with:
```
Traceback (most recent call last):
  File "flang/examples/flang-omp-report-plugin/yaml_summarizer.py", line 58, in <module>
    yaml = YAML()
NameError: name 'YAML' is not defined
```


================
Comment at: flang/examples/flang-omp-report-plugin/yaml_summarizer.py:220-224
+def yaml_module_not_found():
+    """ Print missing yaml module error and exit script. """
+    print("Currently this script only works with\
+           ``ruamel.yaml`` installed.")
+    sys.exit(1)
----------------
[nit] IMHO, we don't need a function for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111042