Skip to content

Conversation

@RussKie
Copy link
Contributor

@RussKie RussKie commented Aug 18, 2021

This reverts commit 5f00dd2.

This broke Visual Basic, that requires implicit usings to work. dotnet/winforms#5437

To double check:

@riarenas
Copy link
Contributor

I'm not sure we should do this. Afaiu, @tmat made this to unbreak a bunch of repos across the stack. Would it be possible to unblock winforms by enabling the feature in the failing projects?

@RussKie
Copy link
Contributor Author

RussKie commented Aug 19, 2021

We absolutely have to do it. First it broke VB that "always" worked. And the implicit usings are being re-done in dotnet/sdk#19521 and dotnet/sdk#19599, and this property is no longer relevant...

@riarenas
Copy link
Contributor

The sdk changes won't make it anywhere until we release one that includes them correct? I'd like to understand how this is a safe change for the other repos.

@RussKie
Copy link
Contributor Author

RussKie commented Aug 20, 2021

dotnet/sdk#19599 was merged a week ago, it should have flown through to target repos.

@dreddy-work
Copy link
Member

ping. All arcade updates into winforms repo are blocked. @tmat .

@mmitche
Copy link
Member

mmitche commented Aug 23, 2021

@RussKie is it possible to conditionalize this on project type so that VB is supported, but C# keeps this behavior?

@RussKie
Copy link
Contributor Author

RussKie commented Aug 23, 2021

@mmitche this property in Q was and is used only by VB targets. It was erroneously reused for C# scenarios in dotnet/sdk#18459, but reverted back in dotnet/sdk#19599. The whole implicit usings has been reimplemented.

IMO Arcade in general shouldn't be used for these kinds of changes. Each repo owner must opt-in or out of specific SDK features.

@mmitche
Copy link
Member

mmitche commented Aug 23, 2021

Alright, I have no-concerns on reverting this.

@RussKie RussKie merged commit 3dd12f0 into main Aug 24, 2021
@RussKie RussKie deleted the revert_5f00dd2_ branch August 24, 2021 23:10
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.

5 participants