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

We use language keywords instead of BCL types #288

Merged
merged 3 commits into from Jan 7, 2019

Conversation

Projects
None yet
6 participants
@MaherJendoubi
Copy link
Contributor

MaherJendoubi commented Jan 3, 2019

Actually, I was inspired by the following PR.

@MaherJendoubi MaherJendoubi requested a review from dotnet/dotnet-winforms as a code owner Jan 3, 2019

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 3, 2019

Looking at dotnet/corefx#391 --it appears that language keywords are preferred

@sharwell
Copy link
Member

sharwell left a comment

Overall I agree with the change. However, two one change is needed:

  1. .editorconfig should be updated to indicate the preference for using language keywords. Edit: this is already completed here:

    winforms/.editorconfig

    Lines 47 to 49 in c04f6f4

    # use language keywords instead of BCL types
    dotnet_style_predefined_type_for_locals_parameters_members = true:suggestion
    dotnet_style_predefined_type_for_member_access = true:suggestion

  2. The commit should be amended so the author of the change is listed as dotnet-bot <dotnet-bot@microsoft.com>

@JuditRose

This comment has been minimized.

Copy link
Member

JuditRose commented Jan 5, 2019

Short summary of changes in this PR:
Object -> object
String -> string
Char -> char
Int16 -> short
Int32 -> int
Int64 -> long
Sbyte -> sbyte
UInt16 ->ushort
UInt32 -> uint
UInt64 -> ulong
Decimal -> decimal
Double -> double
Single -> float

Added lines:
COM2ColorConverter.cs L41: '{' moved to new line
COM2ColorConverter.cs L44: '{' moved to new line
COM2PropertyDescriptor.cs L545: '{' moved to new line
COM2PropertyDescriptor.cs L1445: '{' moved to new line
COM2PropertyDescriptor.cs L1517: '{' moved to new line

LGTM

@MaherJendoubi Thanks for the changes! Could you please take a look at the review comment from @sharwell? Besides that this PR can be merged.

@MaherJendoubi

This comment has been minimized.

Copy link
Contributor

MaherJendoubi commented Jan 5, 2019

@JuditRose Thanks for the recap. I am reading the review.

@MaherJendoubi

This comment has been minimized.

Copy link
Contributor

MaherJendoubi commented Jan 5, 2019

@sharwell Thanks for the review. The commit should be amended so the author of the change is listed as dotnet-bot dotnet-bot@microsoft.com. Is this something that can I do?

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 5, 2019

@MaherJendoubi I'm not sure how to do it from the command line, but I know it's possible from inside Git Extensions. Hopefully someone can provide the specific command.

@wjk

This comment has been minimized.

Copy link

wjk commented Jan 5, 2019

@MaherJendoubi @sharwell

git commit --amend --author="dotnet-bot <dotnet-bot@microsoft.com>"

dotnet-bot and others added some commits Jan 3, 2019

@MaherJendoubi

This comment has been minimized.

Copy link
Contributor

MaherJendoubi commented Jan 5, 2019

@wjk Thanks for the hint.

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 5, 2019

Now someone just needs to force push 5bfd37e to MaherJendoubi:Maher-Jendoubi-Patch-1. 😄

Dismissing my review since the main changes I requested have been mostly addressed and one of the maintainers can follow it to completion before merge.

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 7, 2019

LGTM

@zsd4yr

zsd4yr approved these changes Jan 7, 2019

Copy link
Member

zsd4yr left a comment

Thanks @MaherJendoubi

@zsd4yr zsd4yr merged commit cd433ec into dotnet:master Jan 7, 2019

1 check passed

license/cla All CLA requirements met.
Details

@MaherJendoubi MaherJendoubi deleted the MaherJendoubi:Maher-Jendoubi-Patch-1 branch Jan 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment