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

[Breaking Change] Disable / eliminate RemovedClientParameter breaking change rule #5025

Open
mikekistler opened this issue Dec 22, 2022 · 2 comments
Assignees
Labels
Breaking Changes Improvements to Breaking Changes tooling bug This issue requires a change to an existing behavior in the product in order to be resolved. Central-EngSys This issue is owned by the Engineering System team. Spec PR Tools Tooling that runs in azure-rest-api-specs repo. TypeSpec Issues or feature requests for tooling to support TypeSpec (Cadl)

Comments

@mikekistler
Copy link
Member

The RemovedClientParameter rule is reporting many false positive issues and is unnecessary.

A common example of false positive RemovedClientParameter issues is when a service replaces a parameter definition with a reference to the same parameter in the common-types file. In this case the parameter is not changed -- simply defined elsewhere, so there is no issue.

I think this check is also unnecessary because if the reference to this parameter was changed to something that was not equivalent this would trigger other error messages.

We should eliminate false positives as they can obscure real problems (e.g. by pushing them out of the top 50 shown in the PR report) and waste reviewer time in investigating them.

I think this rule should be disabled for now and considered for complete removal in the near future if there are no unforeseen consequences from disabling it.

@ghost ghost added the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Dec 22, 2022
@mikekistler mikekistler added Spec PR Tools Tooling that runs in azure-rest-api-specs repo. and removed needs-triage This is a new issue that needs to be triaged to the appropriate team. labels Dec 22, 2022
@raych1
Copy link
Member

raych1 commented Jan 11, 2023

@jianyexi , can you investigate?

@jianyexi
Copy link

Agreed, one case is that a 'parameter' is renamed but will not cause the SDK breaking change

@jianyexi jianyexi assigned konrad-jamrozik and unassigned jianyexi Mar 13, 2023
@konrad-jamrozik konrad-jamrozik added Central-EngSys This issue is owned by the Engineering System team. bug This issue requires a change to an existing behavior in the product in order to be resolved. labels Apr 6, 2023
@konrad-jamrozik konrad-jamrozik changed the title Disable / eliminate RemovedClientParameter breaking change rule [Breaking Change] Disable / eliminate RemovedClientParameter breaking change rule May 12, 2023
@mikekistler mikekistler added the Breaking Changes Improvements to Breaking Changes tooling label Jul 7, 2023
@mikekistler mikekistler added the TypeSpec Issues or feature requests for tooling to support TypeSpec (Cadl) label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Changes Improvements to Breaking Changes tooling bug This issue requires a change to an existing behavior in the product in order to be resolved. Central-EngSys This issue is owned by the Engineering System team. Spec PR Tools Tooling that runs in azure-rest-api-specs repo. TypeSpec Issues or feature requests for tooling to support TypeSpec (Cadl)
Projects
Status: 📋 Backlog
Development

No branches or pull requests

4 participants