Skip to content

feat: Introduce TcpClientFactory#555

Open
seilc1 wants to merge 1 commit into
S7NetPlus:mainfrom
seilc1:feature/make-tcp-client-injectable
Open

feat: Introduce TcpClientFactory#555
seilc1 wants to merge 1 commit into
S7NetPlus:mainfrom
seilc1:feature/make-tcp-client-injectable

Conversation

@seilc1

@seilc1 seilc1 commented May 20, 2025

Copy link
Copy Markdown

Allows injecting custom TcpClient for monitoring and logging

@gfoidl gfoidl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

custom TcpClient for monitoring and logging

Seems reasonable to me.


A different approach -- more or less a quite big rewrite -- would be incorporate logging, tracing, and metrics into the lib. But that's a lot of work, and older target frameworks won't profit from that change. So your approach seems to be the better way here.

Comment thread S7.Net/Tcp/ITcpClient.cs Outdated
Comment thread S7.Net/Tcp/TcpClientWrapper.cs Outdated
Comment thread S7.Net/Tcp/TcpClientWrapper.cs Outdated
Comment thread S7.Net/Tcp/TcpClientWrapperFactory.cs Outdated
@seilc1 seilc1 force-pushed the feature/make-tcp-client-injectable branch from 6b30500 to ba1caa5 Compare May 20, 2025 14:15
@seilc1

seilc1 commented May 20, 2025

Copy link
Copy Markdown
Author

custom TcpClient for monitoring and logging

Seems reasonable to me.

A different approach -- more or less a quite big rewrite -- would be incorporate logging, tracing, and metrics into the lib. But that's a lot of work, and older target frameworks won't profit from that change. So your approach seems to be the better way here.

Besides being a bigger rewrite it's also a question of what's needed. I'm not sure if "my" need for logging/metrics would fit a general need or implementation style. So I think it's better to just provide the "possibility".

@seilc1 seilc1 requested a review from gfoidl May 22, 2025 12:22

@gfoidl gfoidl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM -- modulo one comment.

I'd like to hear what the others think about this.

Comment thread S7.Net/Tcp/ITcpClient.cs Outdated
Allows injecting custom TcpClient for monitoring and logging
@seilc1 seilc1 force-pushed the feature/make-tcp-client-injectable branch from ba1caa5 to 34b5f6e Compare May 22, 2025 13:09
@gfoidl

gfoidl commented May 22, 2025

Copy link
Copy Markdown
Collaborator

Please don't force push a branch when a PR is open, that makes reviewing harder, as the last diff disappears.
Just push the normal commit to the branch.

When merging the PR, then (IMO ideally) a squash-merge is done to get the linear history in the main-branch again.

@seilc1

seilc1 commented Jun 2, 2025

Copy link
Copy Markdown
Author

@gfoidl : any idea when your peers will react?

@mycroes

mycroes commented Jun 2, 2025

Copy link
Copy Markdown
Member

@gfoidl : any idea when your peers will react?

Sorry for my late response, awfully busy lately... This seems to be fine, I need to fix the workflows though so GH can also verify it builds. I'll try to get this fixed ASAP.

@seilc1

seilc1 commented Jun 2, 2025

Copy link
Copy Markdown
Author

@gfoidl : any idea when your peers will react?

Sorry for my late response, awfully busy lately... This seems to be fine, I need to fix the workflows though so GH can also verify it builds. I'll try to get this fixed ASAP.

Can I support somhow?

@S7NetPlus S7NetPlus deleted a comment from ynioba Jun 2, 2025
@mycroes

mycroes commented Jun 2, 2025

Copy link
Copy Markdown
Member

@gfoidl : any idea when your peers will react?

Sorry for my late response, awfully busy lately... This seems to be fine, I need to fix the workflows though so GH can also verify it builds. I'll try to get this fixed ASAP.

Can I support somhow?

See PR #557. Unfortunately the unit tests don't run reliably, I'm not 100% sure of the cause, but it might be that the tests execute concurrently thus causing the failures. Feel free to chime in.

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