Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[git-webkit] Add screen-reader friendly review wizard (Part 1) #27628

Conversation

JonWBedard
Copy link
Member

@JonWBedard JonWBedard commented Apr 23, 2024

26ef083

[git-webkit] Add screen-reader friendly review wizard (Part 1)
https://bugs.webkit.org/show_bug.cgi?id=261242
rdar://115083100

Reviewed by Elliott Williams.

Add a read-only text-based PR review tool to git-webkit.

* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:
(Git.__init__): Add an "editor" argument which allows a caller to define
an editor callback which is invoke when the mock commit message editor is
called during the testing context.
* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/__init__.py: Add Review program.
* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/review.py: Added.
(Review.parser): Define arguments for the sub-program.
(Review.editor): Use a repository object to determine the user's prefered editor, matching
the editor used to edit commit messages.
(Review.args_for_url): Extract PR number and repository object from a URL, allowing a
user to specify a PR URL instead of a number and remote.
(Review.invoke_wizard): Generate a local file representing a PR, which includes the PR
diff and comments made against that diff. Open that local file in the user's prefered editor.
(Review.main): Extract a remote pull-request from the provided arguments and invoke the local
wizard with the pull-request details.
* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/review_unittest.py: Added.
(TestReview):
(TestReview.editor_callback):
(TestReview.editor_callback.callback):
(TestReview.test_bitbucket):
(TestReview.test_bitbucket_diff):
(TestReview.test_github):
(TestReview.test_github_files):
(TestReview.test_invalid_pr_url):
(TestReview.test_pr_argument):
(TestReview.test_editor_no_repo):
(TestReview.test_editor_repo):
(TestReview.test_invoke_wizard):
(TestReview.test_help):
(TestReview.test_bitbucket_read):
(TestReview.test_github_read):
(TestReview.test_bitbucket_read_comments):
(TestReview.test_github_read_comments):

Canonical link: https://commits.webkit.org/279244@main

f7983bb

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac   πŸ›  wpe   πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2   πŸ§ͺ wincairo-tests
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac   πŸ§ͺ api-wpe
βœ… πŸ§ͺ webkitpy   πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-cairo
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
  πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2   πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ services   πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress   πŸ§ͺ api-gtk
  πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge   πŸ›  watch-sim

@JonWBedard JonWBedard self-assigned this Apr 23, 2024
@JonWBedard JonWBedard added the Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases label Apr 23, 2024
@JonWBedard JonWBedard changed the title [ [git-webkit] Add screen-reader friendly review wizard Apr 23, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 23, 2024
@JonWBedard JonWBedard removed the merging-blocked Applied to prevent a change from being merged label Apr 24, 2024
@JonWBedard JonWBedard force-pushed the eng/add-screen-reader-friendly-review-wizard branch from eac1a27 to ba0312b Compare April 25, 2024 15:29
@JonWBedard JonWBedard changed the title [git-webkit] Add screen-reader friendly review wizard [ Apr 25, 2024
@JonWBedard JonWBedard changed the title [ [git-webkit] Add screen-reader friendly review wizard (Part 1) Apr 25, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 25, 2024
@JonWBedard JonWBedard removed the merging-blocked Applied to prevent a change from being merged label Apr 30, 2024
@JonWBedard JonWBedard changed the title [git-webkit] Add screen-reader friendly review wizard (Part 1) [ Apr 30, 2024
@JonWBedard JonWBedard force-pushed the eng/add-screen-reader-friendly-review-wizard branch from ba0312b to 364d251 Compare April 30, 2024 20:36
@JonWBedard JonWBedard changed the title [ [git-webkit] Add screen-reader friendly review wizard (Part 1) Apr 30, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 1, 2024
@JonWBedard JonWBedard removed the merging-blocked Applied to prevent a change from being merged label May 10, 2024
@JonWBedard JonWBedard force-pushed the eng/add-screen-reader-friendly-review-wizard branch from 364d251 to ed3950c Compare May 10, 2024 00:32
@JonWBedard JonWBedard marked this pull request as ready for review May 10, 2024 00:33
@JonWBedard
Copy link
Member Author

@emw-apple, there is a "part 2" which is ready as well, I will upload that tomorrow.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 10, 2024
@JonWBedard JonWBedard removed the merging-blocked Applied to prevent a change from being merged label May 10, 2024
@JonWBedard JonWBedard force-pushed the eng/add-screen-reader-friendly-review-wizard branch from ed3950c to 097948b Compare May 13, 2024 18:06
help='Specify remote to search for pull request from.',
)
parser.add_argument(
'--dry-run', '-d', '--no-dry-run',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "-n" is typically the single-character dry run argument.

if from_config:
return from_config.split(' ')

from whichcraft import which
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use shutil.which? Surely we don't need to support python < 3.3 here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not any more! When this PR was first written, we still needed Python 2 support here.

return from_config.split(' ')

from whichcraft import which
return [which('vi')]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: On some linux distributions (iirc, anything Debian-based), vi will be the ancient POSIX vi program, not vim. So you might want to default to vim?

@JonWBedard JonWBedard force-pushed the eng/add-screen-reader-friendly-review-wizard branch from 097948b to f7983bb Compare May 23, 2024 23:37
@JonWBedard JonWBedard added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing skip-ews Applied to prevent a change from being run on EWS labels May 23, 2024
https://bugs.webkit.org/show_bug.cgi?id=261242
rdar://115083100

Reviewed by Elliott Williams.

Add a read-only text-based PR review tool to git-webkit.

* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:
(Git.__init__): Add an "editor" argument which allows a caller to define
an editor callback which is invoke when the mock commit message editor is
called during the testing context.
* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/__init__.py: Add Review program.
* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/review.py: Added.
(Review.parser): Define arguments for the sub-program.
(Review.editor): Use a repository object to determine the user's prefered editor, matching
the editor used to edit commit messages.
(Review.args_for_url): Extract PR number and repository object from a URL, allowing a
user to specify a PR URL instead of a number and remote.
(Review.invoke_wizard): Generate a local file representing a PR, which includes the PR
diff and comments made against that diff. Open that local file in the user's prefered editor.
(Review.main): Extract a remote pull-request from the provided arguments and invoke the local
wizard with the pull-request details.
* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/review_unittest.py: Added.
(TestReview):
(TestReview.editor_callback):
(TestReview.editor_callback.callback):
(TestReview.test_bitbucket):
(TestReview.test_bitbucket_diff):
(TestReview.test_github):
(TestReview.test_github_files):
(TestReview.test_invalid_pr_url):
(TestReview.test_pr_argument):
(TestReview.test_editor_no_repo):
(TestReview.test_editor_repo):
(TestReview.test_invoke_wizard):
(TestReview.test_help):
(TestReview.test_bitbucket_read):
(TestReview.test_github_read):
(TestReview.test_bitbucket_read_comments):
(TestReview.test_github_read_comments):

Canonical link: https://commits.webkit.org/279244@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/add-screen-reader-friendly-review-wizard branch from f7983bb to 26ef083 Compare May 23, 2024 23:52
@webkit-commit-queue
Copy link
Collaborator

Committed 279244@main (26ef083): https://commits.webkit.org/279244@main

Reviewed commits have been landed. Closing PR #27628 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 23, 2024
@webkit-commit-queue webkit-commit-queue merged commit 26ef083 into WebKit:main May 23, 2024
@JonWBedard JonWBedard deleted the eng/add-screen-reader-friendly-review-wizard branch May 24, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-ews Applied to prevent a change from being run on EWS Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
5 participants