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

Wcf receiver client #874

Merged
merged 29 commits into from
Aug 26, 2015
Merged

Conversation

kevindaub
Copy link
Contributor

  • Added back compatibility with 3.2.1 for ILogReceiverClient (missing methods).
  • The base class allows for the code duplication to removed. Also, it allows for the inheriting class to invoke the correct channel from the ClientBase, so WCF should be happy at runtime, too.
  • Added deprecation to ILogReceiverClient and its implementation in favor of the other two interfaces.
  • Added deprecation to the LogReceiverWebServiceTarget's protected method that returned a concrete class in favor of a new method that returns an interface.

This should supersede #821.

  • Verify runtime with WCF
  • Add some unit tests (or run the ones available)
  • Documentation cleanup
  • Check compatibility again
  • Other framework cleanup

304NotModified and others added 28 commits July 20, 2015 00:07
The base class allows for the code duplication to removed.  Also, it
allows for the inheriting class to invoke the correct channel from the
ClientBase<TService>, so WCF should be happy at runtime, too.

Added deprecation to ILogReceiverClient and its implementation in favor
of the other two interfaces.

Added deprecation to the LogReceiverWebServiceTarget's protected method
that returned a concrete class in favor of a new method that returns an
interface.
@304NotModified
Copy link
Member

I'm pretty sure that the generic clientbase won't work. :( See unit tests

@kevindaub
Copy link
Contributor Author

I saw that error. I don't think that's an issue. I've seen errors like that
before. I'll debug it tonight. It looked like we just need a config entry
or modify the custom binding.

On Monday, August 24, 2015, Julian Verdurmen notifications@github.com
wrote:

I'm pretty sure that the generic clientbase won't work. :( See unit tests


Reply to this email directly or view it on GitHub
#874 (comment).

Sent from Gmail Mobile

@304NotModified
Copy link
Member

Well then it Isn't backwards compatible?

@304NotModified
Copy link
Member

@kevindaub
Copy link
Contributor Author

That's not the same error. I'll investigate tonight.

On Monday, August 24, 2015, Julian Verdurmen notifications@github.com
wrote:

http://weblog.west-wind.com/posts/2007/Dec/14/ClientBaseT-and-a-Common-Base-Class


Reply to this email directly or view it on GitHub
#874 (comment).

Sent from Gmail Mobile

Constructor calls were all going to the base constructor.
@kevindaub
Copy link
Contributor Author

Hooray! The issue was the call to the base constructor. The unit tests passed. I was going to look through tonight to see if there are any compatibility issues, but it looks hopeful.

No change to the app.config or unit tests. It was just in the WcfLogReceiverClient constructors.

@304NotModified
Copy link
Member

Good work!

@kevindaub
Copy link
Contributor Author

The documentation looks good to me. Also, the following might be compatibility concerns:

  • WcfLogReceiverClient implements just IWcfLogReceiverClient vs ILogReceiverClient and ClientBase
  • LogReceiverWebServiceTarget uses WcfLogReceiverClient as return in the protected method vs the WcfLogReceiverClientFacade
  • Overall, I think compatibility hits both 3.2.1 and 4.0.1

Lastly, there is a small amount of code duplication for the SilverLight in each of the WcfLogReceiverClient implementations. I think this can be resolved by implementing a similar base class like the WcfLogReceiverClient.

Let me know if there's anything else you need.

304NotModified added a commit that referenced this pull request Aug 26, 2015
@304NotModified 304NotModified merged commit 18417a3 into NLog:master Aug 26, 2015
@304NotModified
Copy link
Member

To be sure:

Also, the following might be compatibility concerns:

  • WcfLogReceiverClient implements just IWcfLogReceiverClient vs ILogReceiverClient and ClientBase

This is a compatibility issue vs (only) NLog 3.2?

  • LogReceiverWebServiceTarget uses WcfLogReceiverClient as return in the protected method vs the WcfLogReceiverClientFacade

This is a compatibility issue vs (only) NLog 4.0?

Overall, I think compatibility hits both 3.2.1 and 4.0.1

Yes, I expected that. But I think we improved the compatibly from 3.2 to 4.1 (4.0 was a mistake)

@kevindaub
Copy link
Contributor Author

  • Both: WcfLogReceiverClient implements just IWcfLogReceiverClient vs ILogReceiverClient and ClientBase
  • Removed the facade and reverted to the 3.2.1. As long as they didn't cast the return value to Clientbase, the compatibility should be pretty close.
    • 4.0.1: Returned WcfLogReceiverClientFacade
    • 3.2.1: Returned WcfLogReceiverClient
    • 4.1.0: Returned WcfLogReceiverClient

@304NotModified
Copy link
Member

Can't we fix the first one with a rename?

@kevindaub
Copy link
Contributor Author

Rename client to facade client?

On Thursday, August 27, 2015, Julian Verdurmen notifications@github.com
wrote:

Can't we fix the first one with a rename?


Reply to this email directly or view it on GitHub
#874 (comment).

Sent from Gmail Mobile

@304NotModified
Copy link
Member

rename IWcfLogReceiverClient to ILogReceiverClient ?

@kevindaub
Copy link
Contributor Author

That won't fix it. It's a binary incompatibility because we changed the
inheritance. But as long as most of the methods of client base are
available. It should be okay.

On Thursday, August 27, 2015, Julian Verdurmen notifications@github.com
wrote:

rename IWcfLogReceiverClient to ILogReceiverClient ?


Reply to this email directly or view it on GitHub
#874 (comment).

Sent from Gmail Mobile

@MartinTherriault
Copy link
Contributor

Finally managed to take a look at this. I tried it in my test scenarios and found no issues.

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.

None yet

3 participants