[PATCH rtems-tools 2/2] misc: Add rtems-style command

[Gedare Bloom]

On Mon, Aug 16, 2021 at 4:14 PM Ida Delphine <idadelm at gmail.com> wrote:
>
> Signed-off-by: Ida Delphine <idadelm at gmail.com>
>
> This command helps to check for style issues or reformat code given a file
> or directory. There are 4 flags:
> * -c, --check : Checks for style issues
> * -r, --reformat : Reformats the code
> * -p, --path : Path to file or dir to be checked or reformatted
> * -i, --ignore : Files to be ignored when checking or reformatting
> * -v, --verbose : Produces a more detailed output.
> ---
>  misc/rtems-style    |  16 +++++
>  misc/tools/style.py | 144 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 160 insertions(+)
>  create mode 100644 misc/rtems-style
>  create mode 100644 misc/tools/style.py
>
> diff --git a/misc/rtems-style b/misc/rtems-style
> new file mode 100644
> index 0000000..5a3e0e8
> --- /dev/null
> +++ b/misc/rtems-style
> @@ -0,0 +1,16 @@
> +#! /usr/bin/env python
> +

See https://docs.rtems.org/branches/master/eng/coding-file-hdr.html#python-file-template

I think this tool should be maybe rtems-c-style? It only focuses on C
and C Header files.

Maybe, later, we need to include rtems-asm-style and rtems-py-style?
Just some thoughts.

> +from __future__ import print_function
> +
> +import sys, os
> +
> +base = os.path.dirname(os.path.dirname(os.path.abspath(sys.argv[0])))
> +rtems = os.path.join(base, 'share', 'rtems')
> +sys.path = sys.path[0:1] + [rtems, base] + sys.path[1:]
> +
> +try:
> +    import misc.tools.style
> +    misc.tools.style.run()
> +except ImportError:
> +    print("Incorrect RTEMS Tools installation", file = sys.stderr)
> +    sys.exit(1)
> \ No newline at end of file
> diff --git a/misc/tools/style.py b/misc/tools/style.py
> new file mode 100644
> index 0000000..a101146
> --- /dev/null
> +++ b/misc/tools/style.py
> @@ -0,0 +1,144 @@
> +import argparse
> +import os
> +import sys
> +import re
> +
> +from rtemstoolkit import execute
> +from rtemstoolkit import git
> +
> +
> +def get_command():
> +    from rtemstoolkit import check
> +
> +    for version in ['', '8', '9', '10', '11']:
> +        if check.check_exe(None, 'clang-format' + version):
> +            command = 'clang-format' + version
> +            return command
> +    print("Clang-format not found in your system")
> +    sys.exit(1)
> +
> +
> +def arguments():
> +    parser = argparse.ArgumentParser(description="Tool for code formatting and style checking \
> +        for RTEMS")
> +    parser.add_argument("-c", "--check", dest="check", help="Check for style differences and \
> +        report the number of issues if found", action="store_true")
> +    parser.add_argument("-r", "--reformat", dest="reformat", help="Reformat the file/directory \
> +        with any style differences found", action="store_true")
> +    parser.add_argument("-p", "--path", dest="path", help="The path to be checked for style issues \
> +        or reformatted")
> +    parser.add_argument("--ignore", dest="ignore", help="Ignore files to be checked or reformatted")
> +    parser.add_argument("-v", "--verbose", dest="verbose", help="A more detailed outline of the \
> +        style issues", action='store_true')
> +    return [parser.parse_args(), parser.print_usage()]
> +
> +
> +

Check the style of your Python code. Make sure it conforms
https://docs.rtems.org/branches/master/eng/python-devel.html

I think we only want two blank lines between function bodies.

> +def get_diff(path, ignore_file=None):
> +    diff = ""
> +    ex = execute.capture_execution()
> +
> +
> +    def clang_to_git_diff(clang_output, path):

Why is this indented to here? I think this changes the visibility of
it to make it a nested function. Is this necessary/desired here?

> +        import os
> +        import tempfile
> +
> +        fd, tmp_path = tempfile.mkstemp()
> +        try:
> +            with os.fdopen(fd, 'w') as tmp:

I think this is trying to open fd again?  Seems wrong, since the fd
file descriptor is already open? I'm not sure.

> +
> +                tmp.write(clang_output)
> +                repo = git.repo(".")

I wonder if we need to provide an argument to the location of the git
repo instead of assume the script runs from within it?

> +            return repo.diff(['--no-index', path, tmp_path])
> +
> +        finally:
> +            os.remove(tmp_path)

Is this all the cleanup needed? what about closing the open fd?

> +
> +    if os.path.isfile(path) == True:
> +        cmd = get_command() + " --style=file " + path
> +        output_clang = ex.command(command=cmd, shell=True)
> +        output_clang = output_clang[2]
> +        diff = clang_to_git_diff(output_clang, path)
> +    else:
> +        onlyfiles = [f for f in os.listdir(path)]
> +        for file in onlyfiles:
> +            file = os.path.join(path, file)
> +
> +            if ignore_file is not None and file == ignore_file:

is there only one ignore_file? Shouldn't there be a set of them? I'm
not sure how this works.

> +                continue
> +
> +            if file.endswith('.c') or file.endswith('.h'):
> +                cmd = get_command() + " --style=file " + file
> +                output_clang = ex.command(command=cmd, shell=True)
> +                output_clang = output_clang[2]
> +                diff += clang_to_git_diff(output_clang, os.path.join(path, file))
> +    return diff
> +
> +
> +def color_text(string, col, style=1):
> +    return f"\033[{style};{col};{col}m{string}\033[0;0m"
> +

I think we need to make sure colored text works well cross-platform.
It might be safer to avoid color in here.

> +
> +def handle_errors(path, output, verbose=False,):
extra comma here?

> +    if len(output) < 1:
> +        print("Everything is clean - No style issues")
> +    else:
> +        print(color_text("Checking for style differences...", 34, style=3))
> +
> +        out = output.split('\n')
> +        files = []
> +        num_diff = 0
> +        for line in out:
> +
> +            if line.startswith("---"):
> +                file = str(re.sub('^---\s[ab]', '', line))
> +                files.append(file)
> +
> +            elif line.startswith('+'):
> +                num_diff += 1
> +                if verbose == True:
> +                    print(color_text(line, 34))
> +                    continue
> +            if verbose == True:
> +                print(line)
> +
> +        print(color_text("\nFiles affected:", 33))
> +
> +        for file in files:
> +            print(file)
> +
> +        message = "\nStyleWarning: You have about a total of " + str(num_diff) + \
> +                " style issue(s) in the " + str(len(files)) + " file(s) listed above"
> +        print(color_text(message, 31))
> +
> +
> +def reformat(path, output):
> +    if len(output) > 0:
> +        onlyfiles = [f for f in os.listdir(path)]
> +        for file in onlyfiles:
> +            if file.endswith('.c') or file.endswith('.h'):
> +                cmd = get_command() + " -i -style=file " + os.path.join(path, file)
> +                ex = execute.capture_execution()
> +                ex.command(command=cmd, shell=True)
> +    else:
> +        print("Everything is clean. No style issues")
> +        return 0
> +
> +
> +def run(args = sys.argv):
> +    args = arguments()
> +    if os.path.exists(args[0].path) == False:
> +        print("Please enter a correct path!!")

It can be helpful to show what was the incorrect path input to help
someone know what they did wrong.

> +        sys.exit(1)
Why exiting instead of returning?

> +    output = get_diff(args[0].path)
> +    if args[0].check == True:
> +        handle_errors(args[0].path, output, verbose=args[0].verbose)
> +    elif args[0].reformat == True:
> +        reformat(args[0].path, output)
> +    else:
> +        args[1]
?

> +        sys.exit(1)
> +
> +
> +if __name__ == "__main__":
> +    run()
> --
> 2.25.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel