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

Merge dependency and project overloads in DependencyFilter #1328

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

Goooler
Copy link
Member

@Goooler Goooler commented Mar 11, 2025

They are merged to accept Any.

Follow up #1322.


  • CHANGELOG's "Unreleased" section has been updated, if applicable.

Sorry, something went wrong.

@Goooler Goooler changed the title Reduce dependency and project overloads in DependencyFilter Reduce dependency and project overloads in DependencyFilter Mar 11, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to reduce dependency and project overloads in the DependencyFilter interface by accepting Any type for dependency and project notations. The changes include modifications to the DependencyFilter interface, its abstract implementation, and corresponding functional tests. Overall, the changes seem reasonable and improve the flexibility of the API.

Summary of Findings

  • Type safety concerns: The change to accept Any for dependency and project notations might introduce runtime errors if the provided object is not of the expected type. Consider adding checks or documentation to mitigate this.
  • Test coverage: The functional tests have been updated to include tests for type-safe dependency accessors. Ensure that all possible scenarios and edge cases are covered.
  • Error handling: The project function in AbstractDependencyFilter throws an error for unsupported notation types. Ensure that this error is properly handled and communicated to the user.

Merge Readiness

The pull request introduces significant changes to the DependencyFilter API. While the changes seem to improve flexibility, there are potential type safety concerns that need to be addressed. I recommend addressing the high severity issue before merging. I am unable to directly approve this pull request, and other reviewers should also review and approve this code before merging.

@Goooler Goooler force-pushed the g/20250311/reduce-overloads-for-dependency-filter branch 3 times, most recently from f721a4f to 06c0330 Compare March 11, 2025 03:39
Base automatically changed from g/20250307/support-type-safe-accessors to main March 11, 2025 03:47
@Goooler Goooler force-pushed the g/20250311/reduce-overloads-for-dependency-filter branch from 06c0330 to 8fd8e04 Compare March 11, 2025 04:06
@Goooler Goooler requested a review from Copilot March 11, 2025 04:07

Choose a reason for hiding this comment

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

PR Overview

This pull request updates the changelog to document the removal of dependency and project overloads in DependencyFilter as part of reducing its dependency acceptability.

  • Updated the "Removed" section in the changelog with a BREAKING CHANGE note.

Reviewed Changes

File Description
docs/changes/README.md Added changelog entry for the removal of overloads.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

They are merged to accept `Any`.
@Goooler Goooler force-pushed the g/20250311/reduce-overloads-for-dependency-filter branch from 8fd8e04 to 6e916b0 Compare March 11, 2025 04:18
@Goooler Goooler changed the title Reduce dependency and project overloads in DependencyFilter Merge dependency and project overloads in DependencyFilter Mar 11, 2025
@Goooler Goooler marked this pull request as ready for review March 11, 2025 09:47
@Goooler Goooler merged commit ce7c148 into main Mar 11, 2025
7 checks passed
@Goooler Goooler deleted the g/20250311/reduce-overloads-for-dependency-filter branch March 11, 2025 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant