[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user RagnarPaulson commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 Thanks Tim, All good feedback. I've cleaned up the comments and white space. Added a new sample, and improved some of the 'toString' methods used in debugging/development. ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user dpauls commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 @tabish121 I appreciate the effort you've put into reviewing this. Just a quick ping to see if there's anything else we can do to get this PR merged in. If you just need to take another look and haven't had time yet, then no worries. We're just eager to see this move towards a release. Thanks! ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user RagnarPaulson commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 Got it. I've just pushed up the latest version. It still passes all the tests using ActiveMQ as a broker, so I believe i got all the namespace changes correct. ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user tabish121 commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 I would just make the changes and amend the commit and force push it to the PR branch, you can also commit things locally and squash later if you prefer that route. Basically until this goes into the project tree there isn't a need to preserve these intermediate code changes in the history ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user RagnarPaulson commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 Thanks Tim, All good feedback. What is the process now? Should I commit my changes as a new commit, or quash it back into the original commit? Ragnar ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user RagnarPaulson commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 Thanks. That seemed to work better. Shows up as one commit now in the pull request. ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user tabish121 commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 If you can't work out the squash then another way to fix things is to back out the commit using a "git reset --soft HEAD~X" where 'X' is the number of commits and then create a new commit from there and push -f to overwrite the PR branch. ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user RagnarPaulson commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 I've tried to follow the quash and rebase instructions outlined https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing But it's not clear to me that this made any difference to the pull request, quite possibly made it worse. ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user tabish121 commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 It is generally best practice to squash commits into one on a PR to make life easier for the reviews and to make the commit history simpler. We don't really need to preserve a commit removing files that shouldn't have been included in the first commit for instance. ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user RagnarPaulson commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 Good catch. In my eagerness to compress several commits into one pull request I lost the .gitignore and inadvertently committed all the binaries/output. I've removed them all now. ---
[GitHub] activemq-nms-amqp issue #2: AMQNET-575: NMS AMQP Client Rework
Github user gemmellr commented on the issue: https://github.com/apache/activemq-nms-amqp/pull/2 I cant really help on the code side as .NET isn't an area I play, but from a skim over the file list for curiosity it seemed like there are various non-code elements in this PR which perhaps shouldn't be included? ---
