Skip to content

Revert "Require partial method signatures to match" (47576)#47879

Merged
cston merged 1 commit intodotnet:masterfrom
cston:revert-47576
Sep 21, 2020
Merged

Revert "Require partial method signatures to match" (47576)#47879
cston merged 1 commit intodotnet:masterfrom
cston:revert-47576

Conversation

@cston
Copy link
Contributor

@cston cston commented Sep 20, 2020

Reverting #47576. Moving the new warning to the 16.9 warning wave.

@cston cston requested a review from a team as a code owner September 20, 2020 23:55
@cston cston added this to the 16.8 milestone Sep 20, 2020
ERR_StaticAnonymousFunctionCannotCaptureThis = 8821,
ERR_OverrideDefaultConstraintNotSatisfied = 8822,
ERR_DefaultConstraintOverrideOnly = 8823,
WRN_PartialMethodTypeDifference = 8824,
Copy link
Member

Choose a reason for hiding this comment

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

I think you mark these with // Available comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could. But this PR simply reverts a previous change so I'll leave as is.

@RikkiGibson
Copy link
Member

Any reason not to simply change the wave to 6 in this PR? Would it be riskier to do so?

@RikkiGibson
Copy link
Member

I just thought if there were a way to keep this functionality behind a flag that .NET 5 users won't encounter, it might make life slightly easier in 16.9, w.r.t. conflicts over error IDs and so on.

@Youssef1313
Copy link
Member

Any reason not to simply change the wave to 6 in this PR? Would it be riskier to do so?

I think this doesn't guarantee that users won't encounter this warning. For example, if they have -warn:9999.

@Youssef1313
Copy link
Member

I just thought if there were a way to keep this functionality behind a flag that .NET 5 users won't encounter, it might make life slightly easier in 16.9, w.r.t. conflicts over error IDs and so on.

Maybe skip the tests add && false to the existing conditions the lead to this warning, with a clarification comment about why it's temporarily disabled. Later on when this is wanted to be enabled again, we need to just remove the extra && false and unskip tests.

@cston
Copy link
Contributor Author

cston commented Sep 21, 2020

Maybe skip the tests add && false to the existing conditions the lead to this warning, with a clarification comment about why it's temporarily disabled. Later on when this is wanted to be enabled again, we need to just remove the extra && false and unskip tests.

It should be easy enough to re-revert this change in 16.9. There will need to be some changes (such as changing the warning wave used in tests), but it shouldn't be difficult. Let's just revert the original change now.

@cston cston requested review from a team and jcouv September 21, 2020 19:05
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@cston cston merged commit c785833 into dotnet:master Sep 21, 2020
@ghost ghost modified the milestones: 16.8, Next Sep 21, 2020
@cston cston deleted the revert-47576 branch September 21, 2020 20:24
@dibarbet dibarbet modified the milestones: Next, 16.8.P4 Sep 21, 2020
Youssef1313 added a commit to Youssef1313/roslyn that referenced this pull request May 12, 2021
333fred added a commit that referenced this pull request Jun 14, 2021
…ures/interpolated-string

* upstream/main: (95 commits)
  Update official build number in separate job
  Update Language Feature Status.md (#54015)
  Remove IRazorDocumentOptionsService inheritance interface (#54047)
  Fix comment
  Simplify
  Do not create a cache field for lambda if it depends on caller's type argument (#44939)
  Documentation
  Documentation
  Documentation
  Update test impls
  Just pass null
  Pull diagnostics should just request from the doc, not the whole project.
  Add test plan for file-scoped namespace (#54003)
  Add source build to official build
  Improved nullable 'is' analysis (#53311)
  Multi session service (#53762)
  Resolve Versions.props conflicts
  Revert "Revert "Require partial method signatures to match" (47576) (#47879)" (#53352)
  Broaden enforcement on prototype marker (#53886)
  Update Language Feature Status.md (#53926)
  ...
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.

5 participants