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

Version 2 #35

Closed
wants to merge 3 commits into from
Closed

Version 2 #35

wants to merge 3 commits into from

Conversation

pbplugge
Copy link

A bit later then said before.
But here is my version.
To be honest i like this version much better, it is clean.
Do with it what you like.

@dhbaird
Copy link
Owner

dhbaird commented Sep 13, 2014

@pbplugge, I am sorry to say that this version will not be merged. I am glad you like it though and hope you are successful with it. I can give you some reasons why it won't be merged, which are completely unacceptable. Please take this feedback with the best intention possible as I hope this leads to you writing better code in the future:

Reason #1: You deleted the pure virtual interface. If you are unfamiliar with object oriented programming, please familiarize yourself immediately with interfaces.

Reason #2: You deleted the Windows port...??! Another contributor graciously worked to add this, and I have absolutely no intention of regressing it.

You were unable to effectively communicate the essence of your design prior to executing the work on the design. Regressing the code as per the above reasons does not constitute an "enhancement." If you want to contribute, I request that you please do so meaningfully. If your goal is to learn about C++, please file tickets along those lines, or contact me personally. This was the wrong way to do it.

@dhbaird dhbaird closed this Sep 13, 2014
@dhbaird dhbaird mentioned this pull request Sep 13, 2014
Repository owner locked and limited conversation to collaborators Sep 13, 2014
@dhbaird
Copy link
Owner

dhbaird commented Sep 13, 2014

I have locked the pull request and your corresponding issue #34 for reference. If you want to continue discussing on this, please file a new ticket.

@dhbaird
Copy link
Owner

dhbaird commented Sep 15, 2014

Here is some more feedback (not withstanding the above feedback, which is even more important):

  • When renaming "send()" to "Send()", please explain the benefit of doing so.
  • Please follow the existing indentation style. This patch re-indents the entire code.

To summarize: smaller patches are much easier to work with. I appreciate your effort to help out, but smaller patches that are easier to discuss and to get right on a case-by-case basis. For a larger patch, there are many more things that can go wrong: Even if you have one good idea (which I believe you probably do have), it unfortunately gets lost due to all the other problems that were introduced.

Edit: I meant to say renaming "send()" to "Send()". I had written "recv()" to "Receive()" but that is what at the moment "poll()" and "dispatch()" do.

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.

None yet

2 participants