search

jakartaee / messaging

Jakarta Messaging
https://eclipse.org/ee4j/messaging
Other
39 stars 32 forks source link

Initial spec doc contribution #267

Closed tivrfoa closed 4 years ago

tivrfoa commented 4 years ago

Hi,

For sure there are things to improve and fix, but I think it's good enough for an initial contribution.

tivrfoa commented 4 years ago

The changes look good except for the main document. The main document is impossible to review as is because the diffs show that everything is new. Did you not check in the original text from the CQ before starting your changes? That makes it hard to tell what you fixed.

Also, if the line length tool is going to touch lots of lines, that will make it hard as well. Line length doesn't look to be a problem in the original contribution so I'm not sure why you think you need that tool.

Also, it look like that tool is introducing some errors. There's a bunch of places where there's a gratuitous space at the beginning of the line and a missing space within the line. Fixing all of those by hand will be a pain.

My advice is to first check in the original text, then apply your changes, and skip the line length tool.

I reset to the commit previous to the changes to the line size.

The content of messaging-spec.adoc in master branch only has a few lines. I then appended the "original" content o the zip file in the commit below: https://github.com/eclipse-ee4j/jms-api/pull/267/commits/7a4143d99dd45b0da7ecfda79aefbcdd0f9c8628

tivrfoa commented 4 years ago

Line length doesn't look to be a problem in the original contribution so I'm not sure why you think you need that tool.

I thought it could be a problem because replacing JMS for Jakarta Messaging introduces 14 new characters.

OndroMih commented 4 years ago

Hi @tivrfoa , @bshannon is right, it would be easier to review if your first committ contained only the original version donated in the CQ and all the other commits with changes are rebased on top of that. I think it can be within the same PR as it's possible to see the changes done in individual commits.

tivrfoa commented 4 years ago

Hi @tivrfoa , @bshannon is right, it would be easier to review if your first committ contained only the original version donated in the CQ and all the other commits with changes are rebased on top of that. I think it can be within the same PR as it's possible to see the changes done in individual commits.

Hi @OndroMih . It was already done like this. Please see this reply: https://github.com/eclipse-ee4j/jms-api/pull/267#issuecomment-606962844

The commit below introduces the original version: Append original specification (Messaging.adoc) to messaging-spec.adoc 7a4143d

The other commits add changes to it.

bshannon commented 4 years ago

How do I see the diff between the original and the updates and then make comments on the updates?

tivrfoa commented 4 years ago

How do I see the diff between the original and the updates and then make comments on the updates?

You are able to comment on each commit. The first commit that changes the original file is this one: https://github.com/eclipse-ee4j/jms-api/pull/267/commits/7a4143d99dd45b0da7ecfda79aefbcdd0f9c8628

OndroMih commented 4 years ago

Thanks, @tivrfoa.

To make it easier to see the differences, I've created a pull request in my forked repo to show the differences between the commit of the original doc and the last commit in this pull request #267. Here you can see the diff: https://github.com/OndroMih/jms-api/pull/1/files, @bshannon

bshannon commented 4 years ago

Why did you end up closing this? Should I no longer review it?

tivrfoa commented 4 years ago

Why did you end up closing this? Should I no longer review it?

Ondo created a new pull request. Thank you! :)

bshannon commented 4 years ago

Where's the new PR?

tivrfoa commented 4 years ago

Where's the new PR?

https://github.com/OndroMih/jms-api/pull/1/files

tivrfoa commented 4 years ago

Where's the new PR?

I think maybe I messed up :/ thought he had created another one ... But that is ok because Ondo has a branch with the changes I made... I thought I could help but I don't know enough. Good luck guys.

bshannon commented 4 years ago

This new PR isn't against the jms-api repo, so when we approve and merge it, it won't change that repo at all. Are we supposed to assume that approving this PR will have the intended effect on the jms-api repo, through some magic? Are you just going to apply all the changes to this fork of jms-api, then apply a PR with all these changes from this forked repo to the jms-api repo?

I think this will work, but you need to explain to your reviewers what it is you doing and what you expect them to do.

OndroMih commented 4 years ago

Hi @tivrfoa, I'm sorry for confusion but my pull request shouldn't replace your pull request. My PR is only for the sake of displaying the differences between the original document (in your first commit) and the document after your changes (your last commit) so that it's easier to review your pull request. I will close my pull request when we're done with reviewing your changes. Please reopen this pull request so that we can continue reviewing it.

OndroMih commented 4 years ago

I've closed my pull request because it's not meant to be merged. It can still be used to see the changes against the original donated document. But we should really reopen this pull request try to reach a point when we can merge it.

bshannon commented 4 years ago

This can't be reopened because the repository from which it was submitted has been deleted. Someone needs to start over with a new pull request. @OndroMih your pull request could be used to create a new pull request, although you'll need to do it "manually".

OndroMih commented 4 years ago

OK, I'll see what I can do.