Multiple features: source path substitution, relative source filename display, custom HTML report title #14
Conversation
There was a problem hiding this comment.
Thanks for submitting a PR @aallrd, this is much appreciated.
I have made a number of comments in your code, could you please address them ?
In particular your code is rightfully rejected by mypy which reports 5 errors. I have mentioned them in my comments, please fix them.
| #### Build and package an executable with `pyinstaller` | ||
|
|
||
| You can use `pyinstaller` to create a single-file executable binary: | ||
|
|
||
| ```bash | ||
| > pip install pyinstaller | ||
| > pyinstaller --onefile --add-data ValgrindCI:ValgrindCI valgrind-ci | ||
| > ./dist/valgrind-ci --help | ||
| ``` | ||
|
|
There was a problem hiding this comment.
What is the point of including this piece of documentation ? Which audience is it intended to ?
| for s in args.substitute_path: | ||
| substitute_paths.append({"from": s.split(":")[0], "to": s.split(":")[1] }) | ||
| data.set_substitute_paths(substitute_paths) |
There was a problem hiding this comment.
s.split(':') is executed twice which is inefficient. Please use local variables instead:
| for s in args.substitute_path: | |
| substitute_paths.append({"from": s.split(":")[0], "to": s.split(":")[1] }) | |
| data.set_substitute_paths(substitute_paths) | |
| for s in args.substitute_path: | |
| _from, _to = s.split(':') | |
| substitute_paths.append({"from": _from, "to": _to}) | |
| data.set_substitute_paths(substitute_paths) |
| if args.relativize: | ||
| prefixes = [] | ||
| for p in args.relativize: | ||
| prefixes.append(p) | ||
| data.set_relative_prefixes(prefixes) |
There was a problem hiding this comment.
Why are you duplicating the list args.relativize in another list prefixes ? Wouldn't it be simpler to send args.relativize as an argument to data.set_relative_prefixes() ?
| if args.relativize: | |
| prefixes = [] | |
| for p in args.relativize: | |
| prefixes.append(p) | |
| data.set_relative_prefixes(prefixes) | |
| if args.relativize: | |
| data.set_relative_prefixes(args.relativize) |
| self._substitute_paths: Optional[List[dict]] = [] | ||
| self._relative_prefixes: Optional[List[str]] = [] |
There was a problem hiding this comment.
Why are these members declared Optional[] ? It doesn't seem to me they can ever be None...
| self._substitute_paths: Optional[List[dict]] = [] | |
| self._relative_prefixes: Optional[List[str]] = [] | |
| self._substitute_paths: List[dict] = [] | |
| self._relative_prefixes: List[str] = [] |
| @@ -130,6 +132,27 @@ def set_source_dir(self, source_dir: Optional[str]) -> None: | |||
| else: | |||
| self._source_dir = None | |||
|
|
|||
| def set_substitute_paths(self, substitute_paths: Optional[List[dict]]) -> None: | |||
| if substitute_paths is not None: | |||
There was a problem hiding this comment.
Can the parameter substitute_paths ever be None ? Why is substitute_paths of type Optional[} ?
| return path | ||
|
|
||
| def relativize(self, path: str) -> str: | ||
| for p in self._relative_prefixes: |
There was a problem hiding this comment.
Mypy reports an error for this line of code: ValgrindCI/parse.py:149: error: Item "None" of "Optional[List[str]]" has no attribute "__iter__" (not iterable) [union-attr]
Please fix it.
| path = path.replace(p, "") | ||
| if path.startswith("/"): | ||
| path = path[1:] |
There was a problem hiding this comment.
Use òs.path.commonprefix()andos.path.relpath()` which are portable across OSes rather than homemade code:
| path = path.replace(p, "") | |
| if path.startswith("/"): | |
| path = path[1:] | |
| if os.path.commonprefix([p, path]) == p: | |
| path = os.path.relpath(path, p) |
| stack["fileref"] = "{}:{}".format(frame_source, error_line) | ||
| fullname = self._data.substitute_path(fullname) | ||
| try: | ||
| with open(fullname, "r", errors="replace") as f: |
There was a problem hiding this comment.
What is the point of using errors="replace" when reading a file ?
| if l >= stack["line"] and l <= error_line + lines_after - 1: | ||
| stack["code"].append(code_line) | ||
| except OSError as e: | ||
| print(f"Warning: cannot read stack data from missing source file: {e.filename}") |
There was a problem hiding this comment.
Shouldn't continue be called when swallowing this exception in order to avoid adding a malformed "stack" to the issue dictionary ?
| print(f"Warning: cannot read stack data from missing source file: {e.filename}") | |
| print(f"Warning: cannot read stack data from missing source file: {e.filename}") | |
| continue |
| src_data, l + 1, lines_before, lines_after | ||
| filename = self._data.substitute_path(filename) | ||
| try: | ||
| with open(filename, "r", errors="replace") as f: |
There was a problem hiding this comment.
There again, what is the point of using errors="replace" when reading a file ?
Hello,
I would like to contribute the below features: