ZedThree / clang-tidy-review

Create a pull request review based on clang-tidy warnings
MIT License
88 stars 44 forks source link

#20 - Fix path replace problem #21

Closed kgfoundrydig closed 2 years ago

kgfoundrydig commented 2 years ago

The problem I was having with your Action was related to the compile_commands.json "directory" entry you make use of on line 350. I'm guessing that you assumed that the directory is always terminated with a /. In my case it is not. This leads to an error later when replacing that directory path which does not exist. I noticed other people saw the same thing, so I'm proposing this simple change to check that you're really removing a /. Works for me now.

ZedThree commented 2 years ago

Lovely, thanks @kgfoundrydig !

I wonder if this might be even clearer as:

        if original_directory.endswith(args.build_dir):
            build_dir_index = -(len(args.build_dir) + 1)
            basedir = original_directory[:build_dir_index]
        elif args.build_dir == ".":
            basedir = original_directory
        else:
            raise RuntimeError(
                f"compile_commands.json contains absolute paths that I don't know how to deal with: '{original_directory}'"
            )

I was probably trying to be clever and avoid assigning to basedir twice, but build_dir_index is only really needed if we need a substring of the directory. Otherwise, we just use the whole thing.

Does this version work for you?

kgfoundrydig commented 2 years ago

Sure. Thanks for listening. Hope this helps.

Edit: yes, I tested it with my config here with success.

ZedThree commented 2 years ago

Awesome, thanks!