`String` changed to `string` #1012

Merged
merged 1 commit into from Dec 16, 2015

Conversation

Projects
None yet
4 participants
@M-Zuber
Contributor

M-Zuber commented Dec 16, 2015

keeping it all the same

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Dec 16, 2015

Contributor

:shipit: This needs a rebase, though. @shiftkey how's the C#6/project.json stuff going? Most of these should be converted to interpolated strings at some point 😄

Contributor

khellang commented Dec 16, 2015

:shipit: This needs a rebase, though. @shiftkey how's the C#6/project.json stuff going? Most of these should be converted to interpolated strings at some point 😄

@M-Zuber

This comment has been minimized.

Show comment
Hide comment
@M-Zuber

M-Zuber Dec 16, 2015

Contributor

I'll do the rebase when my Internet is not from my phone 😁
Does that mean making a new PR? Haven't rebased in a while

Contributor

M-Zuber commented Dec 16, 2015

I'll do the rebase when my Internet is not from my phone 😁
Does that mean making a new PR? Haven't rebased in a while

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Dec 16, 2015

Contributor

There shouldn't be a need to open a new PR. I think the steps should be something along the lines of

  • git checkout uppercase-is-not-always-right // make sure you're on the right branch
  • git remote add upstream git@github.com:octokit/octokit.net.git // if you haven't already, add a new origin to the upstream repository
  • git pull --rebase upstream master // fetch the latest changes from upstream master and rebase on top of it
  • git push -f origin uppercase-is-not-always-right // push the changes
Contributor

khellang commented Dec 16, 2015

There shouldn't be a need to open a new PR. I think the steps should be something along the lines of

  • git checkout uppercase-is-not-always-right // make sure you're on the right branch
  • git remote add upstream git@github.com:octokit/octokit.net.git // if you haven't already, add a new origin to the upstream repository
  • git pull --rebase upstream master // fetch the latest changes from upstream master and rebase on top of it
  • git push -f origin uppercase-is-not-always-right // push the changes
@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Dec 16, 2015

Member

Oh boy. So our convention is something that we can't currently capture with R# rules today (afaik).

  • Do use String when calling static methods of System.String.

ex: String.Format(...)

  • Do use string when specifying a type such as the return type of a method, an argument, a field, a property.

ex: void DoStuff(string foo);

Member

Haacked commented Dec 16, 2015

Oh boy. So our convention is something that we can't currently capture with R# rules today (afaik).

  • Do use String when calling static methods of System.String.

ex: String.Format(...)

  • Do use string when specifying a type such as the return type of a method, an argument, a field, a property.

ex: void DoStuff(string foo);

@M-Zuber

This comment has been minimized.

Show comment
Hide comment
@M-Zuber

M-Zuber Dec 16, 2015

Contributor

NP.
You guys where not around in gitter and @shiftkey seemed to not know this twitter.

I will close this and reopen a new PR that adds to the documentation. What is the target file? Contributing.md?

Contributor

M-Zuber commented Dec 16, 2015

NP.
You guys where not around in gitter and @shiftkey seemed to not know this twitter.

I will close this and reopen a new PR that adds to the documentation. What is the target file? Contributing.md?

@M-Zuber

This comment has been minimized.

Show comment
Hide comment
@M-Zuber

M-Zuber Dec 16, 2015

Contributor

What about new string?

Contributor

M-Zuber commented Dec 16, 2015

What about new string?

@M-Zuber M-Zuber closed this Dec 16, 2015

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Dec 16, 2015

Member

What about new string?

Do we do that anywhere? 😛

The general principle I have in my head is that when we're using System.String like a primitive, we use the C# shorthand syntax. Hence string for return types, etc.

When we use it like a type, I like it to look like a type. Hence String.Format, String.Empty, etc.

Hence I'd prefer new String() or new Int32() over new string() and new int(). Anyone know if we can configure R# or VS rules to follow that convention? Or am I being too picky?

Member

Haacked commented Dec 16, 2015

What about new string?

Do we do that anywhere? 😛

The general principle I have in my head is that when we're using System.String like a primitive, we use the C# shorthand syntax. Hence string for return types, etc.

When we use it like a type, I like it to look like a type. Hence String.Format, String.Empty, etc.

Hence I'd prefer new String() or new Int32() over new string() and new int(). Anyone know if we can configure R# or VS rules to follow that convention? Or am I being too picky?

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Dec 16, 2015

Contributor

Anyone know if we can configure R# or VS rules to follow that convention? Or am I being too picky?

Nope. It's either the keyword or the CLR type. Keyword is default 😄

Contributor

khellang commented Dec 16, 2015

Anyone know if we can configure R# or VS rules to follow that convention? Or am I being too picky?

Nope. It's either the keyword or the CLR type. Keyword is default 😄

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Dec 16, 2015

Member

I wrote a blog post about the convention. I'm seriously considering changing my mind given that the corefx team has their guidelines and to be quite honest, I hate fighting defaults and I hate arguing every single convention.

I'd rather have a one-time argument where we decide on a set of conventions someone else maintains and when people complain about any given convention, we tell them to take it up with the dotnet team. 😛

Member

Haacked commented Dec 16, 2015

I wrote a blog post about the convention. I'm seriously considering changing my mind given that the corefx team has their guidelines and to be quite honest, I hate fighting defaults and I hate arguing every single convention.

I'd rather have a one-time argument where we decide on a set of conventions someone else maintains and when people complain about any given convention, we tell them to take it up with the dotnet team. 😛

@M-Zuber

This comment has been minimized.

Show comment
Hide comment
@M-Zuber

M-Zuber Dec 16, 2015

Contributor

While I have come around to @Haacked's way of thinking in this regard,
having a set of guidelines that is not our fault can be very useful.

One point that has not been mentioned is people new to C#. It may be confusing to them as in a lot of cases the existence of the BCL types is not even known to them. Sticking to the keywords may be less confusing.

I can reopen the PR if you so desire

Contributor

M-Zuber commented Dec 16, 2015

While I have come around to @Haacked's way of thinking in this regard,
having a set of guidelines that is not our fault can be very useful.

One point that has not been mentioned is people new to C#. It may be confusing to them as in a lot of cases the existence of the BCL types is not even known to them. Sticking to the keywords may be less confusing.

I can reopen the PR if you so desire

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Dec 16, 2015

Member

I can reopen the PR if you so desire

Sure. I'd love to hear @shiftkey's thoughts on this.

Member

Haacked commented Dec 16, 2015

I can reopen the PR if you so desire

Sure. I'd love to hear @shiftkey's thoughts on this.

@M-Zuber

This comment has been minimized.

Show comment
Hide comment

@M-Zuber M-Zuber reopened this Dec 16, 2015

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Dec 16, 2015

Member

Okay, so here's my thoughts on this:

  • String.Method definitely feels more Type-y, and I do empathise with the mental "jump" that occurs if you come across a string.Method
  • For other types that have aliases in C# (long, int, etc) I don't really recall having the same hassle - in fact, I kinda squirm if i see Int64 or Int32 mentioned explicitly.
  • The using statement arguments are kinda trivial for me - there's likely other things in the System namespace which you'll need, aside from String.

For the sake of consistency, I'm 👍 on moving it all to string.

Member

shiftkey commented Dec 16, 2015

Okay, so here's my thoughts on this:

  • String.Method definitely feels more Type-y, and I do empathise with the mental "jump" that occurs if you come across a string.Method
  • For other types that have aliases in C# (long, int, etc) I don't really recall having the same hassle - in fact, I kinda squirm if i see Int64 or Int32 mentioned explicitly.
  • The using statement arguments are kinda trivial for me - there's likely other things in the System namespace which you'll need, aside from String.

For the sake of consistency, I'm 👍 on moving it all to string.

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Dec 16, 2015

Member

@khellang

how's the C#6/project.json stuff going?

Given we're all on VS2015 now, this is possibly something worth entertaining (although there might be work to bring the Mono side up to parity once #995 lands).

Member

shiftkey commented Dec 16, 2015

@khellang

how's the C#6/project.json stuff going?

Given we're all on VS2015 now, this is possibly something worth entertaining (although there might be work to bring the Mono side up to parity once #995 lands).

@Haacked

This comment has been minimized.

Show comment
Hide comment
Member

Haacked commented Dec 16, 2015

selfie-0

Haacked added a commit that referenced this pull request Dec 16, 2015

@Haacked Haacked merged commit b9eeed3 into octokit:master Dec 16, 2015

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@M-Zuber

This comment has been minimized.

Show comment
Hide comment
@M-Zuber

M-Zuber Dec 16, 2015

Contributor

😂 That sums up the thread so perfectly.
Thank you for the patience.

Contributor

M-Zuber commented Dec 16, 2015

😂 That sums up the thread so perfectly.
Thank you for the patience.

@M-Zuber

This comment has been minimized.

Show comment
Hide comment
@M-Zuber

M-Zuber Dec 16, 2015

Contributor

@shiftkey can I open a WIP for the string.format work?

Contributor

M-Zuber commented Dec 16, 2015

@shiftkey can I open a WIP for the string.format work?

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Dec 16, 2015

Member

@M-Zuber I'll open an issue after merging the Mono PR

Member

shiftkey commented Dec 16, 2015

@M-Zuber I'll open an issue after merging the Mono PR

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