Skip to content

Code fix for CS1615 (Remove 'in' keyword)#44529

Merged
CyrusNajmabadi merged 3 commits intodotnet:masterfrom
eugenesmlv:remove-in-keyword
Jun 11, 2020
Merged

Code fix for CS1615 (Remove 'in' keyword)#44529
CyrusNajmabadi merged 3 commits intodotnet:masterfrom
eugenesmlv:remove-in-keyword

Conversation

@eugenesmlv
Copy link
Contributor

@eugenesmlv eugenesmlv commented May 23, 2020

Closes #44187

I'm not sure about handling 'in' keyword's trivia. Should we keep it and prepend to the argument or just remove the keyword with its trivia?

@eugenesmlv eugenesmlv requested a review from a team as a code owner May 23, 2020 20:28
@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request labels May 26, 2020
M2(in i, s);
}
}");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have negative tests showing we don't remove in when it should be kept around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like M2 method in this case?

@"class Class
{
void M1(int i) { }
void M2(in int i, string s) { }
void N1(int i)
{
M1(in {|FixAllInDocument:i|});
}
void N2(int i, string s)
{
M2(in i, in s);
}
void N3(int i, string s)
{
M1(in i);
M2(in i, in s);
}
}",

Or do you mean it would be nice to add more tests for that particular case?

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd like a test htat purely verifies the negative scenario and nothing else :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added :)

<comment>'using' is a C# keyword and should not be localized</comment>
</data>
<data name="Remove_in_keyword" xml:space="preserve">
<value>Remove 'in' keyword</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

check out the other resource strings. There should be a way to mark in as locked. this will tell the loc team to not localize that word when they do the different languages.

if (argumentSyntax == null || argumentSyntax.GetRefKind() != RefKind.In)
return;

var generator = context.Document.GetRequiredLanguageService<SyntaxGenerator>();
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line. you can get the generation inside FixAsync.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

LGTM with the loc change asked for.

@CyrusNajmabadi
Copy link
Contributor

@eugenesmlv Is this ready for a final review?

@eugenesmlv
Copy link
Contributor Author

eugenesmlv commented Jun 11, 2020

Yes, it is. Is there anything I can improve in this PR?

@CyrusNajmabadi CyrusNajmabadi merged commit 67dfdee into dotnet:master Jun 11, 2020
@ghost ghost added this to the Next milestone Jun 11, 2020
@CyrusNajmabadi
Copy link
Contributor

Thanks!

@eugenesmlv eugenesmlv deleted the remove-in-keyword branch June 11, 2020 19:19
@dibarbet dibarbet modified the milestones: Next, 16.7.P4 Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Code fix for CS1615 (Remove 'in' keyword)

4 participants