Skip to content

Conversation

@AdamSpeight2008
Copy link
Contributor

Fix for Issue #27189

@AdamSpeight2008 AdamSpeight2008 requested a review from a team as a code owner May 26, 2018 16:12
@sharwell
Copy link
Contributor

I prefer static readonly to const for non-private fields in C# code, but this is Visual Basic and also other people on the team may disagree. Before we make a change here let me talk to the team on Monday to reach a conclusion.

@sharwell sharwell added Area-IDE Need Design Review The end user experience design needs to be reviewed and approved. labels May 26, 2018
@sharwell sharwell added this to the 15.8 milestone May 26, 2018
@sharwell sharwell self-assigned this May 26, 2018
@AdamSpeight2008
Copy link
Contributor Author

@sharwell I prefer Const in this context, as the value of those string are highly unlikely to change anytime soon. IMHO. It's here if needed.

Friend Const Main As String = "vb.Main"
Friend Const Async As String = "vb.Async"
Friend Const Iterator As String = "vb.Iterator"
Friend Const Await As String = "vb.Await"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side Note: The colour of Await and Yield are incorrect, should be black in this context.

Friend Shared Iterator As String = "vb.Iterator"
Friend Shared Await As String = "vb.Await"
Friend Shared Yield As String = "vb.Yield"
Friend Const Assign As String = "vb.Assign"
Copy link
Member

Choose a reason for hiding this comment

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

can we use shared-readonly instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

➡️ As I mentioned above, this is currently the subject of an open design discussion which we'll make a decision on and report back. To avoid unnecessary back and forth it didn't make sense to request a change until the design review reaches a conclusion.

@etbyrd etbyrd added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 26, 2018
@sharwell
Copy link
Contributor

sharwell commented Jun 5, 2018

This went to a design meeting yesterday. Since the impacted code did not fall into the public API surface (even though IVTs are potentially impacted), we did not reach a final conclusion regarding an expected pattern future developers are supposed to follow.

We therefore reviewed this as a refactoring operation without clear guidance on the 'one right way' to proceed. Since the old code unexpectedly defined the fields as mutable, we felt that a change to Const is a clear improvement to the overall code quality and thus justifies the inclusion of the change even though we don't have specific guidance.

@sharwell sharwell removed the Need Design Review The end user experience design needs to be reviewed and approved. label Jun 5, 2018
@jinujoseph
Copy link
Contributor

Approved to merge for 15.8.Preview3

@sharwell sharwell merged commit b27a3e4 into dotnet:master Jun 5, 2018
@AdamSpeight2008
Copy link
Contributor Author

@sharwell What does IVTs mean? as it a new acronym to me.

@jinujoseph
Copy link
Contributor

IVT => Internal Visible True

@AdamSpeight2008 AdamSpeight2008 deleted the patch-1 branch June 7, 2018 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved to merge Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Concept-Continuous Improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants