Bug 220180 - Remove some unused variables from webkitpy
Summary: Remove some unused variables from webkitpy
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
Keywords: InRadar
Depends on:
Reported: 2020-12-27 17:24 PST by Alexey Proskuryakov
Modified: 2021-01-05 13:09 PST (History)
8 users (show)

See Also:

proposed patch (54.00 KB, patch)
2020-12-27 17:28 PST, Alexey Proskuryakov
sam: review+
jbedard: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2020-12-27 17:24:28 PST
Address some of the more obvious linter warnings.
Comment 1 Alexey Proskuryakov 2020-12-27 17:28:41 PST
Created attachment 416826 [details]
proposed patch
Comment 2 Radar WebKit Bug Importer 2021-01-03 17:25:12 PST
Comment 3 Aakash Jain 2021-01-04 09:12:04 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=416826&action=review


> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:158
>            test_dir: absolute path to the LayoutTests directory.

Should delete this line as well.

> Tools/Scripts/webkitpy/tool/commands/upload.py:-511
> -        bug_id = tool.bugs.create_bug(bug_title, comment_text, options.component, diff, "Patch", cc=options.cc, mark_for_review=options.review, mark_for_commit_queue=options.request_commit)

Maybe we could add a log statement here to log the bug_id (either user visible or at DEBUG level). Not sure how helpful that would be.
Comment 4 Jonathan Bedard 2021-01-05 08:28:13 PST
Comment on attachment 416826 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416826&action=review

I don't think we should be changing some of the style checker ones without just removing the classes in question

> Tools/Scripts/webkitpy/style/checkers/jsonchecker.py:34
> +    def __init__(self, handle_style_error):

If this class isn't accepting a file path, what is it even doing? Seems like the better approach on this one would be to make it actually work, which I think would be be done by reading the file in, then printing the json with an indent of 4 and comparing.

> Tools/Scripts/webkitpy/style/checkers/watchlist.py:40
> +    def __init__(self, handle_style_error):

If this class isn't accepting a file path, what is it even doing?

> Tools/Scripts/webkitpy/style/checkers/xml.py:34
>          self._handle_style_error = handle_style_error

If this class isn't accepting a file_path, what is it even doing?

> Tools/Scripts/webkitpy/tool/steps/suggestreviewers_unittest.py:48
> +        OutputCapture()

Should just remove this entirely.
Comment 5 Alexey Proskuryakov 2021-01-05 10:18:04 PST
> If this class isn't accepting a file path, what is it even doing?

These checkers use the lines argument, and don't need a file path. There can be an argument for keeping a uniform interface for all checkers though.

I'm going to revert this part for now, because it's also incorrect - JSONChecker has subclasses that are still created with a file path argument, and are thus completely broken by this patch. And there are zero unit tests for any of them, so test-webkitpy didn't tell me about the mistake.
Comment 6 Alexey Proskuryakov 2021-01-05 10:36:38 PST
Committed https://trac.webkit.org/r271158
Comment 7 Ryan Haddad 2021-01-05 12:42:35 PST
This broke `run-builtins-generator-tests`:

Traceback (most recent call last):
  File "./Tools/Scripts/run-builtins-generator-tests", line 48, in <module>
  File "./Tools/Scripts/run-builtins-generator-tests", line 45, in main
    return BuiltinsGeneratorTests(reset_results, executive.Executive()).main()
  File "/Volumes/Data/slave/catalina-release-tests-wk2/build/Tools/Scripts/webkitpy/codegen/main.py", line 161, in main
    if not self.run_tests(input_directory, reference_directory):
  File "/Volumes/Data/slave/catalina-release-tests-wk2/build/Tools/Scripts/webkitpy/codegen/main.py", line 148, in run_tests
    if not self.run_test(reference_directory, "WebCoreJSBuiltins.h", separately_generated_files, self.wrappers_builtin_test):
  File "/Volumes/Data/slave/catalina-release-tests-wk2/build/Tools/Scripts/webkitpy/codegen/main.py", line 114, in run_test
    if generate_builtin_callback(test_name, test_files, work_directory):
TypeError: wrappers_builtin_test() takes exactly 3 arguments (4 given)

Comment 8 Ryan Haddad 2021-01-05 13:09:21 PST
(In reply to Ryan Haddad from comment #7)
> This broke `run-builtins-generator-tests`
Reverted the relevant part of r271158 in r271173.