Summary

Don't call handle_file() if a patched file can't be fetched.

Review Request #11547 — Created March 22, 2021 and submitted March 25, 2021, 4:12 p.m.

Information

Owner

chipx86

Repository

ReviewBot

Branch

release-3.0.x

Bugs

Depends On

Reviewers

Groups

reviewbot

People

Description

All tools that implement handle_file() were calling

get_patched_file_path() on the reviewed file, and returning early if

it was None. Rather than do this in each handle_file() method, it

makes more sense to do it in handle_files(), before calling

handle_file().
This change switches to that, and passes in the resulting path as a

path variable, so handle_file() can operate on it.
We don't perform the check or pass in path for legacy tools, since the

old handle_file() didn't accept **kwargs and wouldn't expect it.
To handle the legacy tool check, the old base classes now set a

legacy_tool attribute, which the inner class can check. This is

admittedly pretty hacky, but won't exist past Review Bot 3.0, and is

easier to maintain than forking these methods into the old base classes.

Testing Done

Unit tests pass on Python 2.7 and 3.x.

Commits

Summary	ID
Don't call handle_file() if a patched file can't be fetched. All tools that implement `handle_file()` were calling `get_patched_file_path()` on the reviewed file, and returning early if it was `None`. Rather than do this in each `handle_file()` method, it makes more sense to do it in `handle_files()`, before calling `handle_file()`. This change switches to that, and passes in the resulting path as a `path` variable, so `handle_file()` can operate on it. We don't perform the check or pass in `path` for legacy tools, since the old `handle_file()` didn't accept `**kwargs` and wouldn't expect it. To handle the legacy tool check, the old base classes now set a `legacy_tool` attribute, which the inner class can check. This is admittedly pretty hacky, but won't exist past Review Bot 3.0, and is easier to maintain than forking these methods into the old base classes.	bd11340cade03698c6ec60bd1a34e92cfe0422d6

Summary

Don't call handle_file() if a patched file can't be fetched.

All tools that implement `handle_file()` were calling `get_patched_file_path()` on the reviewed file, and returning early if it was `None`. Rather than do this in each `handle_file()` method, it makes more sense to do it in `handle_files()`, before calling `handle_file()`. This change switches to that, and passes in the resulting path as a `path` variable, so `handle_file()` can operate on it. We don't perform the check or pass in `path` for legacy tools, since the old `handle_file()` didn't accept `**kwargs` and wouldn't expect it. To handle the legacy tool check, the old base classes now set a `legacy_tool` attribute, which the inner class can check. This is admittedly pretty hacky, but won't exist past Review Bot 3.0, and is easier to maintain than forking these methods into the old base classes.

bd11340cade03698c6ec60bd1a34e92cfe0422d6

Issues

Description	From	Last Updated
Can you add documentation for legacy_tool?	david	March 25, 2021, 1:58 a.m.

flake8 passed.

JSHint passed.

Show all issues

Can you add documentation for legacy_tool?

chipx86 March 25, 2021, 1:56 a.m.

I can. It's really just internal state that shouldn't ever be touched or set directly.

Change Summary:

Added docs for the legacy_tool attribute, telling people not to touch it.

Commits:

	Summary	ID
	Don't call handle_file() if a patched file can't be fetched. All tools that implement `handle_file()` were calling `get_patched_file_path()` on the reviewed file, and returning early if it was `None`. Rather than do this in each `handle_file()` method, it makes more sense to do it in `handle_files()`, before calling `handle_file()`. This change switches to that, and passes in the resulting path as a `path` variable, so `handle_file()` can operate on it. We don't perform the check or pass in `path` for legacy tools, since the old `handle_file()` didn't accept `**kwargs` and wouldn't expect it. To handle the legacy tool check, the old base classes now set a `legacy_tool` attribute, which the inner class can check. This is admittedly pretty hacky, but won't exist past Review Bot 3.0, and is easier to maintain than forking these methods into the old base classes.	60d51500b4369d5f8d1da347842103ddd3433217
	Don't call handle_file() if a patched file can't be fetched. All tools that implement `handle_file()` were calling `get_patched_file_path()` on the reviewed file, and returning early if it was `None`. Rather than do this in each `handle_file()` method, it makes more sense to do it in `handle_files()`, before calling `handle_file()`. This change switches to that, and passes in the resulting path as a `path` variable, so `handle_file()` can operate on it. We don't perform the check or pass in `path` for legacy tools, since the old `handle_file()` didn't accept `**kwargs` and wouldn't expect it. To handle the legacy tool check, the old base classes now set a `legacy_tool` attribute, which the inner class can check. This is admittedly pretty hacky, but won't exist past Review Bot 3.0, and is easier to maintain than forking these methods into the old base classes.	bd11340cade03698c6ec60bd1a34e92cfe0422d6

Diff:

Revision 2 (+206 -22)

Show changes

	bot/reviewbot/tests/test_tasks.py
	bot/reviewbot/tools/__init__.py
	bot/reviewbot/tools/pmd.py
	bot/reviewbot/tools/base/tool.py
	bot/reviewbot/tools/tests/test_base_tool.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (cb7d553)