Skip to content

Conversation

@shs96c
Copy link
Member

@shs96c shs96c commented Dec 11, 2024

Future updates to rules_closure remove the handy closure/library target we depend on. Use exact deps instead.

Future updates to `rules_closure` remove the handy `closure/library`
target we depend on. Use exact deps instead.
@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Dependencies
The all_js_for_testing target has empty deps but includes all JS files. Should verify if any closure library dependencies are needed for the test files.

Missing Dependencies
The all_js_for_testing target has empty deps but includes all JS files. Should verify if any closure library dependencies are needed for the test files.

@qodo-merge-pro
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
General
Add missing browser specification attribute to ensure consistent fragment configuration

Add missing browsers = ["chrome"] attribute to the get-location-in-view fragment to
match the pattern used in get-location fragment above it.

javascript/chrome-driver/BUILD.bazel [36-37]

 closure_fragment(
     name = "get-location-in-view",
+    browsers = ["chrome"],
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: Adding the browser specification maintains consistency with other fragments and explicitly declares the intended browser target, which is important for build configuration clarity and maintainability.

7
Remove unnecessary empty dependency list to improve build file clarity

Remove empty deps list since it's not needed when there are no dependencies.

javascript/webdriver/BUILD.bazel [43]

 visibility = ["//javascript:__pkg__"],
-deps = [],
  • Apply this suggestion
Suggestion importance[1-10]: 3

Why: While technically correct, removing an empty deps list has minimal impact on functionality. It's a minor cleanup that slightly improves code clarity.

3

@shs96c shs96c merged commit 2cc530d into SeleniumHQ:trunk Dec 12, 2024
10 of 11 checks passed
@shs96c shs96c deleted the rules-closure-bump-strict-deps branch December 12, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant