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

Feature/asciidocs #1207

Closed
wants to merge 66 commits into from
Closed

Conversation

vladmihalcea
Copy link
Contributor

The architecture and the domain model chapters are ready for review. Looking forward to getting your feedback.

- Database_Access.adoc
- Transactions.adoc
- JNDI.adoc
- Locking.adoc
- Fetching.adoc
- Batching.adoc
- Caching.adoc
- Events.adoc
@vladmihalcea
Copy link
Contributor Author

The tests (54) could go into one commit, for instance. Those are located in the hibernate-entitymanager (hibernate-entitymanager/src/test/java/org/hibernate/jpa/guide).

I could remove the css and the images folder under asciidoc (148) files since we only use the default Asciidoctor theme for now.

Otherwise, plain git would probably display all modified files.

@sebersole
Copy link
Member

Yes, I think all the "theme" files should have not been moved yet. Just leave them in their original place until (if) we actually need them. No idea about these "tests".

The issue is not the ability to "display all modified files" btw. The issue is being able to comment on what needs to be changed. That's what a PR is for, for discussion. If the GitHub UI cannot render all these files then we cannot discuss via line comments in those files obviously.

@vladmihalcea
Copy link
Contributor Author

From the Help guide:

The maximum number of files in a single diff is limited to 300.

I'll have to see how I can split that.

@vladmihalcea
Copy link
Contributor Author

I counted all files under the Userguide and there are 410 and counting. Basically there are two options:

  • I commit only the asciidocs and then everything else
  • We use regular git diffing and use other medium for exchanging comments

@sebersole
Copy link
Member

The other option is to simply integrate all this work upstream and start from there as a baseline in terms of "review"

@sebersole
Copy link
Member

But by and large the "distinction" of all these commits is organic to your workflow, but not so in terms of the overall task. At the end of the day, the user will not care less that this rewrite was done in 5 commits or in 500; it's completely irrelevant.

IMO we ought to push this work as of some specific commit you chose, and then begin the review/editing from there. That lets us focus on more atomic edits.

@vladmihalcea
Copy link
Contributor Author

I have done much progress with the review of the remaining chapters, so I got to Chapter 13. HQL. Although I added more TODO sections, I think it's worth if I review all those to make sure the migration was fine and then we can push them in one big commit and continue the review from there. Afterwards there will be less files to be reviewed anyway.

@sebersole
Copy link
Member

In terms of your initial review, sure.

But I have already found quite a few edits. Some are pretty large
changes. The problem for me is how to track these edits I propose. At the
moment I simply cannot. I need some known point from which to start. So
then when you are ready to push, we will push this work upstream and I will
start my review from the start yet again. I will not review any more until
that point, because its pointless. I have to start from scratch every time
and its a huge waste of my time right now until we get that known point.

On Wed, Jan 6, 2016 at 10:50 AM Vlad Mihalcea notifications@github.com
wrote:

I have done much progress with the review of the remaining chapters, so I
got to Chapter 13. HQL. Although I added more TODO sections, I think it's
worth if I review all those to make sure the migration was fine and then we
can push them in one big commit and continue the review from there.
Afterwards there will be less files to be reviewed anyway.


Reply to this email directly or view it on GitHub
#1207 (comment)
.

@vladmihalcea
Copy link
Contributor Author

Ok, I'll let you know when the remaining chapters are reviewed and we can push it upstream

@vladmihalcea vladmihalcea deleted the feature/asciidocs branch January 14, 2016 15:11
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.

2 participants