This repository has been archived by the owner. It is now read-only.

Add include and exclude attributes to EnvironmentTagHelper #5722

Merged
merged 4 commits into from Jan 27, 2017

Conversation

Projects
None yet
7 participants
@jbagga
Copy link
Contributor

jbagga commented Jan 27, 2017

Addresses #5671

cc @Eilon

@@ -30,13 +34,33 @@ public EnvironmentTagHelper(IHostingEnvironment hostingEnvironment)

/// <summary>
/// A comma separated list of environment names in which the content should be rendered.
/// If the current environment is also in the <see cref="Exclude"/> list, the content will not be rendered.
/// </summary>
/// <remarks>
/// The specified environment names are compared case insensitively to the current value of
/// <see cref="IHostingEnvironment.EnvironmentName"/>.
/// </remarks>
public string Names { get; set; }

This comment has been minimized.

@ajaybhargavb

ajaybhargavb Jan 27, 2017

Member

@DamianEdwards @Eilon, do we plan to remove this in the future? If so should we add a remark saying this is deprecated?

This comment has been minimized.

@DamianEdwards

DamianEdwards Jan 27, 2017

Member

Nah, I'd just leave it. It's basically an alias for Include

This comment has been minimized.

@NTaylorMullen

NTaylorMullen Jan 27, 2017

Member

What if we made it a true alias for Include? Aka, have its getter/setter point to Include. That way we could add a doc comment stating it was an alias which would move people towards Include and avoid any confusion in the future.

It'd also simplify the code in process a decent amount.

This comment has been minimized.

@ajaybhargavb

ajaybhargavb Jan 27, 2017

Member

That still won't restrict people from using both at the same time and that will cause some confusion.

This comment has been minimized.

@Eilon

Eilon Jan 27, 2017

Member

Yeah keep both properties. It's a bit gross, but the alternatives seem worse.

/// </summary>
public class EnvironmentTagHelper : TagHelper
{
private static readonly char[] NameSeparator = new[] { ',' };
private static readonly char[] IncludeEnvironmentsSeparator = new[] { ',' };
private static readonly char[] ExcludeEnvironmentsSeparator = new[] { ',' };

This comment has been minimized.

@dougbu

dougbu Jan 27, 2017

Member

Why imply we'd ever use different separators for Include and Exclude? Can use NameSeparator everywhere.

if (hasExcludeEnvironments)
{
// No matching exclude environment name found, do nothing
return;
}

This comment has been minimized.

@dougbu

dougbu Jan 27, 2017

Member

Why does this block exist? Method is about to return anyhow.

Also do not need the return at line 157 or the hasExcludeEnvironments variable.

This comment has been minimized.

@Eilon

Eilon Jan 27, 2017

Member

I partially agree with this comment. I think the return at line 157 is important because it ensures the method will end right there and then. It will prevent a future change later in this method from inadvertently running.

}
}
}

var hasEnvironments = false;

This comment has been minimized.

@dougbu

dougbu Jan 27, 2017

Member

Just need one hasIncludeEnvironments variable for both Names and Includes.

[InlineData("Development", "Test", "", "Development")]
[InlineData("Development", "Development", "Test", "Development")]
[InlineData("Development", "Test", "Test", "Development")]
[InlineData("Test", "Development", "Test", "Development")]

This comment has been minimized.

@dougbu

dougbu Jan 27, 2017

Member

Test null as well as empty strings. Same for the data added below. (Note: You don't really need to test every positive "", "Development", "Test" combination.)

Also suggest removing the environmentName parameter since its value never varies.

This comment has been minimized.

@ajaybhargavb

ajaybhargavb Jan 27, 2017

Member

Also include a few cases to ensure that the comparison is case insensitive

var output = MakeTagHelperOutput("environment", childContent: content);
var hostingEnvironment = new Mock<IHostingEnvironment>();
hostingEnvironment.SetupProperty(h => h.EnvironmentName);
hostingEnvironment.Object.EnvironmentName = environmentName;

This comment has been minimized.

@dougbu

dougbu Jan 27, 2017

Member

Why is this line needed?

This comment has been minimized.

@jbagga

jbagga Jan 27, 2017

Contributor

If the EnvironmentName is not set, it's null and always returns from the null check for currentEnvironmentName

This comment has been minimized.

@dougbu

dougbu Jan 27, 2017

Member

Something is busted with mocking if you need both of these lines. The "real" EnvironmentName property should never be called.

This comment has been minimized.

@jbagga

jbagga Jan 27, 2017

Contributor

hostingEnvironment.SetupProperty(h => h.EnvironmentName, "Development"); seems to work. Should I modify all the older tests to do the same?

This comment has been minimized.

@dougbu

dougbu Jan 27, 2017

Member

Yes please. Much less head-scratching for all.

@dougbu

This comment has been minimized.

Copy link
Member

dougbu commented Jan 27, 2017

Overall comment: Need a couple of tests covering comma-separated values for Include and `Exclude.

@@ -11,10 +11,14 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers
/// <summary>
/// <see cref="ITagHelper"/> implementation targeting &lt;environment&gt; elements that conditionally renders
/// content based on the current value of <see cref="IHostingEnvironment.EnvironmentName"/>.
/// If the environment is not listed in explicitly identified <see cref="Names"/> or <see cref="Include"/> lists or

This comment has been minimized.

@NTaylorMullen

NTaylorMullen Jan 27, 2017

Member

If the environment is not listed in <see cref="Names"/> or <see cref="Include"/>, or if it is in <see cref="Exclude"/>, the content will not be rendered.

This comment has been minimized.

@jbagga

jbagga Jan 27, 2017

Contributor

This is a little tricky. If the user does not specify Names, Include or Exclude, the content is rendered for all environments. But if they do specify Names and/or Include, but the current environment is not in it, then it is suppressed.

Maybe
If the environment is not listed in the specified <see cref="Names"/> or <see cref="Include"/>, or if it is in <see cref="Exclude"/>, the content will not be rendered.

@@ -30,13 +34,33 @@ public EnvironmentTagHelper(IHostingEnvironment hostingEnvironment)

/// <summary>
/// A comma separated list of environment names in which the content should be rendered.
/// If the current environment is also in the <see cref="Exclude"/> list, the content will not be rendered.

This comment has been minimized.

@NTaylorMullen

NTaylorMullen Jan 27, 2017

Member

you have an extra space after <see cref="Exclude"/>.

@@ -30,13 +34,33 @@ public EnvironmentTagHelper(IHostingEnvironment hostingEnvironment)

/// <summary>
/// A comma separated list of environment names in which the content should be rendered.
/// If the current environment is also in the <see cref="Exclude"/> list, the content will not be rendered.
/// </summary>
/// <remarks>
/// The specified environment names are compared case insensitively to the current value of
/// <see cref="IHostingEnvironment.EnvironmentName"/>.
/// </remarks>
public string Names { get; set; }

This comment has been minimized.

@NTaylorMullen

NTaylorMullen Jan 27, 2017

Member

What if we made it a true alias for Include? Aka, have its getter/setter point to Include. That way we could add a doc comment stating it was an alias which would move people towards Include and avoid any confusion in the future.

It'd also simplify the code in process a decent amount.

@ajaybhargavb
Copy link
Member

ajaybhargavb left a comment

⌚️

/// </summary>
public class EnvironmentTagHelper : TagHelper
{
private static readonly char[] NameSeparator = new[] { ',' };
private static readonly char[] IncludeEnvironmentsSeparator = new[] { ',' };

This comment has been minimized.

@ajaybhargavb

ajaybhargavb Jan 27, 2017

Member

I don't see any reason to have these separate. Maybe just rename NameSeparator to something generic and use the same for include and exclude.

This comment has been minimized.

@ajaybhargavb

ajaybhargavb Jan 27, 2017

Member

Just realized @dougbu already gave this feedback 🙈

if (environment.HasValue && environment.Length > 0)
{
hasExcludeEnvironments = true;
if (environment.Equals(currentEnvironmentName))

This comment has been minimized.

@ajaybhargavb

ajaybhargavb Jan 27, 2017

Member

StringComparison.OrdinalIgnoreCase

if (environment.HasValue && environment.Length > 0)
{
hasIncludeEnvironments = true;
if (environment.Equals(currentEnvironmentName))

This comment has been minimized.

@ajaybhargavb

ajaybhargavb Jan 27, 2017

Member

StringComparison.OrdinalIgnoreCase

@@ -30,13 +34,33 @@ public EnvironmentTagHelper(IHostingEnvironment hostingEnvironment)

/// <summary>
/// A comma separated list of environment names in which the content should be rendered.
/// If the current environment is also in the <see cref="Exclude"/> list, the content will not be rendered.
/// </summary>
/// <remarks>
/// The specified environment names are compared case insensitively to the current value of
/// <see cref="IHostingEnvironment.EnvironmentName"/>.
/// </remarks>
public string Names { get; set; }

This comment has been minimized.

@Eilon

Eilon Jan 27, 2017

Member

Yeah keep both properties. It's a bit gross, but the alternatives seem worse.

public string Include { get; set; }

/// <summary>
/// A comma separated list of environment names in which the content should not be rendered.

This comment has been minimized.

@Eilon

Eilon Jan 27, 2017

Member

Replace should with will. In fact, make this change in all the comments (assuming it applies 😄 ).

This comment has been minimized.

@jbagga

jbagga Jan 27, 2017

Contributor

It applies for Exclude. But for Include and Names, if the environment is also in Exclude, it will not be rendered. Leaving those as "should"

This comment has been minimized.

@Eilon

Eilon Jan 27, 2017

Member

Makes sense, thanks!

if (hasExcludeEnvironments)
{
// No matching exclude environment name found, do nothing
return;
}

This comment has been minimized.

@Eilon

Eilon Jan 27, 2017

Member

I partially agree with this comment. I think the return at line 157 is important because it ensures the method will end right there and then. It will prevent a future change later in this method from inadvertently running.

@jbagga

This comment has been minimized.

Copy link
Contributor

jbagga commented Jan 27, 2017

@dougbu

dougbu approved these changes Jan 27, 2017

Copy link
Member

dougbu left a comment

:shipit: from my perspective but get @Eilon's sign-off on the updated doc comments.

@@ -66,6 +66,95 @@ public void ShowsContentWhenCurrentEnvironmentIsNotSet(string namesAttribute, st
ShouldShowContent(namesAttribute, environmentName);
}


This comment has been minimized.

@dougbu

dougbu Jan 27, 2017

Member

nit: Remove extra line

@jbagga jbagga force-pushed the jbagga/EnvironmentTagHelper5671 branch from f160f29 to fb1d6ec Jan 27, 2017

@ajaybhargavb
Copy link
Member

ajaybhargavb left a comment

:shipit:

}
}
}

if (hasEnvironments)
{
// This instance had at least one non-empty environment specified but none of these
// This instance had at least one non-empty environment (name or include) specified but none of these

This comment has been minimized.

@ajaybhargavb

ajaybhargavb Jan 27, 2017

Member

name => names

@Eilon

Eilon approved these changes Jan 27, 2017

@jbagga jbagga merged commit 7449ffa into dev Jan 27, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@jbagga jbagga deleted the jbagga/EnvironmentTagHelper5671 branch Jan 31, 2017

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