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

Make HTML helper null handling consistent #934

Merged
merged 1 commit into from
Aug 6, 2014
Merged

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Aug 4, 2014

nits:

  • correct summary XML comment for HtmlHelper class
  • use named parameters and prefer interface (not extension) methods in changed calls

@dougbu
Copy link
Member Author

dougbu commented Aug 5, 2014

Rebased and made a small addition to the commit description. Otherwise unchanged.

dougbu added a commit that referenced this pull request Aug 5, 2014
- #847 line 7 sub-bullets
- don't call extension methods from our templates
 - partially addressed in PR #934 (for methods changed there)
- don't reference extension methods from XML comments
@NTaylorMullen
Copy link
Member

:shipit:

@@ -231,7 +231,7 @@ public interface IHtmlHelper
/// </summary>
/// <param name="name">The name of the HTML element.</param>
/// <returns>The ID of the HTML element.</returns>
string GenerateIdFromName(string name);
string GenerateIdFromName([NotNull] string name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Going back to #765, this might not make sense. The presence of [NotNull] on an interface doesn't guarantee the implementation would have a NotNull.

Copy link
Member Author

Choose a reason for hiding this comment

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

👎 We're not fixing #765 right now and the rest of this interface uses [NotNull]. We should be consistent.

In any case those uses aren't really about guaranteeing an implementation has the same attribute (though a compiler extension or analysis tool that did that would be nice as well). More about complaints an analysis tool might make if it notices a potentially-null string passed to this method.

@pranavkm
Copy link
Contributor

pranavkm commented Aug 6, 2014

:shipit:

- #874 lines 3, 4, and 6
- correct `Value()` to treat a `null` expression name the same as `string.Empty`
- add missing `[NotNull]` attributes in `EditorExtensions` and for `GenerateIdFromName()`
- consistently pass `null` for default expression names to the helpers
 - for example, from extension methods
- add test cases using `null` for expression name

nits:
- correct summary XML comment for `HtmlHelper` class
- use named parameters and prefer interface (not extension) methods in changed calls
- use `string.Empty` instead of `""` in a few tests
@dougbu dougbu merged commit 56d66c0 into dev Aug 6, 2014
dougbu added a commit that referenced this pull request Aug 6, 2014
- #847 line 7 sub-bullets
- don't call extension methods from our templates
 - partially addressed in PR #934 (for methods changed there)
- don't reference extension methods from XML comments
@dougbu dougbu deleted the helper.null.and.notnull.847 branch August 7, 2014 00:10
dougbu added a commit that referenced this pull request Aug 7, 2014
- correct XML comment typo introduced in `HtmlHelper` in 56d66c0 (bad merge)
- fix missed `null` requirement for `@Html.RadioButtonFor()` and remove buried `null` check
- add back `Environment.Newline` to `@Html.TextArea()` (was dropped though comment wasn't)
dougbu added a commit that referenced this pull request Aug 7, 2014
- correct XML comment typo introduced in `HtmlHelper` in 56d66c0 (bad merge)
- fix missed `null` requirement for `@Html.RadioButtonFor()` and remove buried `null` check
- add back `Environment.Newline` to `@Html.TextArea()` (was dropped though comment wasn't)
dougbu added a commit that referenced this pull request Aug 8, 2014
- correct XML comment typo introduced in `HtmlHelper` in 56d66c0 (bad merge)
- fix missed `null` requirement for `@Html.RadioButtonFor()` and remove buried `null` check
- add back `Environment.Newline` to `@Html.TextArea()` (was dropped though comment wasn't)
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.

3 participants