Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.

Conversation

sornaks
Copy link

@sornaks sornaks commented Feb 23, 2015

#296. Looking for comments around naming.

@ghost
Copy link

ghost commented Feb 23, 2015

Hi @sornaks, I'm your friendly neighborhood Microsoft Open Technologies, Inc. Pull Request Bot (You can call me MSOTBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Sornakumar Sundararajan). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla2.msopentech.com.

TTYL, MSOTBOT;

@ghost ghost added the cla-not-required label Feb 23, 2015
@sornaks sornaks force-pushed the Output branch 4 times, most recently from d0a071c to 3ff2c13 Compare February 23, 2015 18:24

Choose a reason for hiding this comment

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

throw this in the previous if statement as an ||

Copy link
Contributor

Choose a reason for hiding this comment

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

"that is appended to the content" -> "to be appended" (use consistent wording)

@NTaylorMullen
Copy link

⌚ for #312 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

teensy nit: in future don't remove the trailing initializer commas. you're more likely to get nits to add them.

@dougbu
Copy link
Contributor

dougbu commented Mar 4, 2015


in future please do not squash commits before reviewers are done

@NTaylorMullen
Copy link

:shipit: once @dougbu is happy.

Copy link
Member

Choose a reason for hiding this comment

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

should we skip adding the value if it's null? would be consistent with Append(TagHelperContent)

@sornaks
Copy link
Author

sornaks commented Mar 4, 2015

Updated..

Copy link
Contributor

Choose a reason for hiding this comment

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

!string.IsNullOrEmpty() (almost-always weird to just null-check a string)

Copy link
Author

Choose a reason for hiding this comment

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

But why would you want to ignore string.Empty?

@dougbu
Copy link
Contributor

dougbu commented Mar 4, 2015

:shipit: with !string.IsNullOrEmpty() fix

@dougbu
Copy link
Contributor

dougbu commented Mar 4, 2015

:shipit:

@sornaks
Copy link
Author

sornaks commented Mar 4, 2015

Checked in. Thanks guys! bd9d57d

@sornaks sornaks closed this Mar 4, 2015
@sornaks sornaks deleted the Output branch March 11, 2015 17:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants