[f-string 1/2] Remove mylog formatting strings

[cphyc]

PR Summary

Replaces all occurrence of mylog.info("Some information message about %s %s" % ("foo", "bar")) by mylog.info("Some information message about %s %s", "foo", "bar")

PR Checklist

• pass black --check yt/
• pass isort . --check --diff
• pass flake8 yt/
• revert insane loglevel to initial value once tests pass

Extra information

This relies on the following code to detect the occurences:

import argparse
import ast
import os
import re
from glob import glob

parser = argparse.ArgumentParser()
parser.add_argument("folder", type=str, default=".")
parser.add_argument("-v", "--verbose", action="store_true")


class MyLogVisitor(ast.NodeVisitor):
    def __init__(self, filename, source):
        self.filename = filename
        self.source = source

    def visit_Call(self, node):
        if (
            isinstance(node, ast.Call)
            and isinstance(node.func, ast.Attribute)
            and hasattr(node.func.value, "id")
            and node.func.value.id == "mylog"
            and node.func.attr in ("debug", "info", "warn", "error", "warning", "critical")
            and not isinstance(node.args[0], ast.Str)
        ):
            lstart = max(0, node.lineno - 2)
            lend = min(len(self.source), node.lineno + 2)
            print(f"{self.filename}:{node.lineno}:{node.col_offset} error")

        self.generic_visit(node)


def main():
    args = parser.parse_args()

    # Find filenames
    if os.path.isfile(args.folder):
        filenames = [args.folder]
    else:
        filenames = []
        for root, dirs, files in os.walk(args.folder):
            if '__pycache__' in root:
                continue
            for fn in files:
                if fn.endswith('.py'):
                    path = os.path.join(root, fn)
                    filenames.append(path)
        filenames = sorted(filenames)

    # Find filenames
    for fn in filenames:
        if args.verbose:
            print(f"Analysing {fn}")
        with open(fn) as f:
            line_by_line = f.readlines()
            f.seek(0)
            lines = f.read()
        try:
            tree = ast.parse(lines, fn)
        except SyntaxError as e:
            print(e)
        mlv = MyLogVisitor(fn, line_by_line)
        mlv.visit(tree)


if __name__ == "__main__":
    main()

If you're using vs code, you can also add a problem detector using

{
    // See https://go.microsoft.com/fwlink/?LinkId=733558
    // for the documentation about the tasks.json format
    "version": "2.0.0",
    "tasks": [
        {
            "label": "find bad mylog calls",
            "type": "shell",
            "command": "python /path/to/find_mylog_errors.py .",
            "group": {
                "kind": "build",
                "isDefault": true
            },
            "isBackground": false,
            "problemMatcher": {
                "owner": "python",
                "fileLocation": ["relative", "${workspaceFolder}"],
                "pattern": {
                  "regexp": "^(.*):(\\d+):(\\d+) \\s+(warning|error)$",
                  "file": 1,
                  "line": 2,
                  "column": 3,
                  "severity": 4
                }
              }
        }
    ]
}

[neutrinoceros]

Looks like this would enable solving #2757 with a mere call to flynt yt/ !!! Love it.

[neutrinoceros]

Thanks for including the detection script, this allowed me to catch this line

            and node.func.attr in ("debug", "info", "warn", "error")

where you forgot to include the fifth and ultimate logging level "critical" ! Also "warning" exists and I'm pretty sure it's still used in yt (though it's a deprecated alias for "warn", see https://docs.python.org/3/library/logging.html#logging.Logger.warning)

edit: my bad, it's the other way around, warn is the deprecated one. It was addressed in #2285, but we still have 4 occurrences of it in the code base (two of which are in the RAMSES frontend 😛 )

[cphyc]

Thanks for including the detection script, this allowed me to catch this line

            and node.func.attr in ("debug", "info", "warn", "error")

where you forgot to include the fifth and ultimate logging level "critical" ! Also "warning" exists and I'm pretty sure it's still used in yt (though it's a deprecated alias for "warn", see https://docs.python.org/3/library/logging.html#logging.Logger.warning)

edit: my bad, it's the other way around, warn is the deprecated one. It was addressed in #2285, but we still have 4 occurrences of it in the code base (two of which are in the RAMSES frontend stuck_out_tongue )

Good catch, and you're right that I missed a handful of them!

[neutrinoceros]

mylog.critical is actually never used atm, so it doesn't make a difference actually, but I see you caught a lot more anyway, well done !

[neutrinoceros]

oops, this is on me, thanks !

[matthewturk]

Awesome

[cphyc]

Note: this was not used anywhere.

[cphyc]

@matthewturk @neutrinoceros before merging this, do you think we could set the default logging level to 40 (and then revert it) so that all logs are visited and we're sure nothing is going wrong?

[neutrinoceros]

sure ! may as well go for 50 ! :-) (disregard if I'm mistaken, I think this is the maximal meaningful level)

[matthewturk]

I think it is! Let's try it.

…

On Thu, Aug 6, 2020 at 9:34 AM Clément Robert ***@***.***> wrote: sure ! may as well go for 50 ! :-) (disregard if I'm mistaken, I *think* this is the maximal meaningful level) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2820 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXOZDYQ2F4WUZY7X4FHDR7K5QLANCNFSM4PWSH2BA> .

[neutrinoceros]

Found two remaining unjustified strings concatenations, otherwise all good ! Thanks again for doing this !

[cphyc]

I'm converting to draft until the tests are done, after that I'll revert 19f78f1 and that'll be good to go!

[cphyc]

Good to go now!

[neutrinoceros]

Awesome ! There's a huge queue on CI runner right now, so I'll happily merge this in 10 to 12 hours when it clears.
For immediate reference, is this the travis job that ran with all log entries activated ?
https://tests.yt-project.org/job/yt_py38_git/928/

[cphyc]

Yes ; it ran successfully before reverting the commit that was (temporarily) setting the default log value to 50.