Skip to content
This repository was archived by the owner on Sep 2, 2021. It is now read-only.

Reduced public API#13

Merged
timbussmann merged 7 commits intomasterfrom
reduced-public-api
Dec 13, 2016
Merged

Reduced public API#13
timbussmann merged 7 commits intomasterfrom
reduced-public-api

Conversation

@Scooletz
Copy link
Copy Markdown
Contributor

@Scooletz Scooletz commented Dec 9, 2016

All types made internal with only two public extension methods to configure the routing.

PoA:

  • seal public API leaving only extension methods
  • provide a test for custom configuration paths

Addresses #11

@Scooletz Scooletz self-assigned this Dec 9, 2016
@Scooletz
Copy link
Copy Markdown
Contributor Author

Scooletz commented Dec 9, 2016

Ping @timbussmann @SzymonPobiega
master merged to this branch. Ready to review.

@Scooletz Scooletz changed the title [WIP] Reduced public API Reduced public API Dec 9, 2016
<Reference Include="System.Xml" />
</ItemGroup>
<ItemGroup>
<Compile Include="Properties\AssemblyInfo.cs" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I need it for InternalsVisibleTo for tests project.

</ItemGroup>
<ItemGroup />
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<!-- To modify your build process, add your task inside one of the targets below and uncomment it.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔥

@@ -0,0 +1,37 @@
using System.Reflection;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔥 it all

@Scooletz
Copy link
Copy Markdown
Contributor Author

Scooletz commented Dec 9, 2016

@SzymonPobiega you need to be more explicit than 🔥 :P I use AssemblyInfo for
InternalsVisibleTo. Do you know any better way of providing them?

@SzymonPobiega
Copy link
Copy Markdown
Contributor

@Scooletz I meant, most of the assembly info stuff is rendundant - generated by GitVersion. So leave only InternalsVisibleTo there?

/// Enables routing configured with the routing configuration file.
/// </summary>
/// <param name="config">The configuration object.</param>
public static void EnableFileBasedRouting(this EndpointConfiguration config)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we could also consider disabling autosubscribe in case it's a transport without native pubsub features

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we throw an exception if somebody wants to use centralized routing for events with a transport that supports native event routing?


namespace NServiceBus.FileBasedRouting
{
public static class FileBasedRoutingConfig
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's common to postfix the class containing extensions methods with Extensions

}

await Endpoint.Create(endpointConfiguration); // to init all features
var routingTable = endpointConfiguration.GetSettings().Get<UnicastRoutingTable>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we name this integration test and rely on internal behavior which can easily change, I think we should instead consider using the acceptance testing framework and start up matching endpoints and do the tests that way. But this isn't a blocker for this PR, I think we should do that in a dedicated test anyway. Thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quite a lot of ceremony in order to validate if we can pass a configuration parameter properly. I've seen this problem a number of times where we need to spin up an endpoint to validate if we are passing values through settings correctly. I'd 🔥 this test provided it was at least once observed to be green -- not likely to break in future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's keep them in here as long as they don't cause any pain since they have been written now anyway. But good to 🔥 them immediately on some sign of issues

var route = UnicastRoute.CreateFromEndpointName(endpoint.LogicalEndpointName);
foreach (var commandType in endpoint.Commands)
{
if (nativeSends)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that could probably be addressed in a more elegant way by #16

@timbussmann timbussmann merged commit 63ba214 into master Dec 13, 2016
@timbussmann timbussmann deleted the reduced-public-api branch December 13, 2016 10:53
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