FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: Introduce TcpClientFactory by seilc1 · Pull Request #555 · S7NetPlus/s7netplus · GitHub

Repository navigation

feat: Introduce TcpClientFactory - #555

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

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

Conversation

seilc1 commented May 20, 2025

Copy link
Copy Markdown

Allows injecting custom TcpClient for monitoring and logging

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the feature/make-tcp-client-injectable branch from 6b30500 to ba1caa5 Compare May 20, 2025 14:15

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 requested a review from gfoidl May 22, 2025 12:22

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the feature/make-tcp-client-injectable branch from ba1caa5 to 34b5f6e Compare May 22, 2025 13:09

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 commented Jun 2, 2025

Copy link
Copy Markdown
Author

@gfoidl : any idea when your peers will react?

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 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 deleted a comment from ynioba Jun 2, 2025

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL