Skip to content
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

change ContextExtensionMethods namespace #97

Merged
merged 1 commit into from
Feb 16, 2018

Conversation

brandonh-msft
Copy link
Contributor

@brandonh-msft brandonh-msft commented Feb 12, 2018

The ContextExtensions should be in Microsoft.Bot.Builder namespace; in order to use them, users need to see them. They don't see them if they have to import a completely separate namespace. In JS SDK, this hurdle doesn't exist; shouldn’t in .Net.

@brandonh-msft
Copy link
Contributor Author

cc @cleemullins

Copy link
Contributor

@cleemullins cleemullins left a comment

Choose a reason for hiding this comment

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

The namespace of a file generally matches it's path. In this case, the path includes a /ContextExtensions/ segment. That means the namespace should match. Exceptions are certainly possible, but this doesn't seem like it's warranted.

Your comment calls out the typical C# Behavior:

in order to use them, users need to see them. They don't see
them if they have to import a completely separate namespace.

Typically in C#, developers DO import the namespaces to see extension methods. That way they only see what they want. That's pretty much what we've been doing to date.

The namespace and approach you see were deliberate.

For reference, here's the relevant C# "Docs":

One simple way to improve the robustness of your extension method library is to put extension methods into their own namespace. This is the strategy employed by the 3.5 version of the .NET Framework. By putting extension methods into their own namespace you enable consumers to include or exclude them separately from the rest of your library. This makes them pluggable, allowing users to easily replace extension method implementations with other implementations if they wish (which is at the heart of the LINQ provider model). This in turn then makes it easier for them to resolve any conflicts that may arise (see part 2 for information about extension method precedence levels).

Of course, the end of the docs there then say:

Please ignore this advice where it is not appropriate

Also note, the docs I'm reading are ancient. If best practices have evolved, please point me at them and I'll update accordingly.

To further confuse the issues, here's recent discussions on StackOverflow that are totally unhelpful! :)

In short, I'm open to be persuaded.

@brandonh-msft
Copy link
Contributor Author

brandonh-msft commented Feb 14, 2018

just to reiterate, this is actual customer feedback. They still don't think that C# has SendTyping and Delay due to this issue.

The ContextExtensions should be in Microsoft.Bot.Builder namespace; in order to use them, users need to see them. They don't see them if they have to import a completely separate namespace. In JS SDK, this hurdle doesn't exist; shouldn’t in .Net.
Copy link
Contributor

@tomlm tomlm left a comment

Choose a reason for hiding this comment

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

:shipit:

@tomlm tomlm merged commit 1ca112a into microsoft:master Feb 16, 2018
cleemullins added a commit that referenced this pull request Feb 16, 2018
Tests no longer pass after PR #97. The ContextExtension name space ch…
@brandonh-msft brandonh-msft deleted the patch-1 branch February 21, 2018 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants