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.

Sign up for GitHub

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

Jump to bottom

Convert the doc format to Markdown for en #2784

Merged
merged 19 commits into from
Apr 9, 2023
Merged

Convert the doc format to Markdown for en #2784

merged 19 commits into from
Apr 9, 2023

Conversation

awxiaoxian2020
Copy link
Contributor

@awxiaoxian2020 awxiaoxian2020 commented Jan 31, 2023 •
edited
Loading

Part of #2691

@coveralls
Copy link

coveralls commented Jan 31, 2023 •
edited
Loading

Coverage Status

Coverage: 87.545%. Remained the same when pulling 06fae43 on awxiaoxian2020:convert-md-en into e219318 on mybatis:master.

@awxiaoxian2020 awxiaoxian2020 mentioned this pull request Jan 31, 2023
Closed
@awxiaoxian2020
Copy link
Contributor Author

I will start for other languages when this PR is reviewed and merged.

@awxiaoxian2020
Copy link
Contributor Author

Welcome to review the format!

@harawata
Copy link
Member

harawata commented Feb 4, 2023

As I suspected, I cannot use common diff tools for the review. 🥲
I really don't have time to review this without diff-ing the results.
If someone knows a smart DOM diff tool for the task, please let me know.
Or, if there is someone who is willing to review this without a diff tool, that would be appreciated.

@awxiaoxian2020
Copy link
Contributor Author

Hi, @harawata
I can tell you how I make it.

For example I open the website https://mybatis.org/mybatis-3/getting-started.html and copy the all content, then I paste it to Typora, which can render the contents to Markdown most correctly. And I adjust the content for some format which is not rendered. It's a pity that the software is not free, but I only want to tell you the convertion could be trusted mostly.

harawata
harawata requested changes Feb 4, 2023
View reviewed changes
Copy link
Member

@harawata harawata left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just reviewed the Configuration page.
Please double check other files using the following method.

  1. Install HTML Tidy.
  2. Create a text file tidy-config.txt 
    wrap: 0
new-pre-tags: code
new-inline-tags: code
indent: auto
  3. Execute mvn site with the original xdoc files.
  4. Create a tidy-ed version of a HTML file. 
    tidy -o configuration-xdoc.html -config tidy-config.txt target/site/configuration.html
  5. Execute mvn site with the markdown files.
  6. Create a tidy-ed version of a HTML file. 
    tidy -o configuration-markdown.html -config tidy-config.txt target/site/configuration.html
  7. Compare the two tidy-ed version of HTMLs using a diff tool (I used macOS's FileMerge.app), but any decent diff tool should be fine.

src/site/markdown/configuration.md Outdated Show resolved Hide resolved
src/site/markdown/configuration.md Show resolved Hide resolved
src/site/markdown/configuration.md Show resolved Hide resolved
src/site/markdown/configuration.md Outdated Show resolved Hide resolved
src/site/markdown/configuration.md Show resolved Hide resolved
src/site/markdown/configuration.md Show resolved Hide resolved
src/site/markdown/configuration.md Show resolved Hide resolved
src/site/markdown/configuration.md Show resolved Hide resolved
@hazendaz
Copy link
Member

hazendaz commented Feb 4, 2023

would it also be far easier to keep the original for now and just add the new so it can come into code base sooner rather than continuing to sit un-merged? Think we are all in same boat here, not enough time to review and its difficult with the original is whacked at the same time. And its likely to take some deal of tweaking as seen here already. I think if we kept original, this just comes in and tweaking should then continue over time until we all feel comfortable the old can go away. My two cents anyways...

@harawata
Copy link
Member

harawata commented Feb 4, 2023 •
edited
Loading

@hazendaz ,
I didn't know that was an option.
So, xdoc has precedence over markdown when generating the site?
And does that mean we need to edit both xdoc and markdown when updating the docs?

I actually think it's best to convert the same file for all languages in one PR (i.e. xdoc/configuraiton.html, ja/xdoc/configuration.html, ...) because we copy the English version to the other languages when we modify documentation (and that was why I asked to choose one file first).

Anyway, we don't edit docs very often, so if having both xdoc and md makes your work easier, I am totally OK. :)

@hazendaz
Copy link
Member

hazendaz commented Feb 4, 2023 via email

@awxiaoxian2020
Copy link
Contributor Author

awxiaoxian2020 commented Feb 4, 2023 •
edited
Loading

Not sure the pom settings needed. Site should already filter. If not need to add that to the parent.

The pom settings needed indeed. I agree it that we add it to the parent. We must handle the escape filter first in all mybatis project and then add it to parent. Now just keep as here and I will handle with escape filter in code @hazendaz

@awxiaoxian2020
Copy link
Contributor Author

I actually think it's best to convert the same file for all languages in one PR (i.e. xdoc/configuraiton.html, ja/xdoc/configuration.html, ...) because we copy the English version to the other languages when we modify documentation (and that was why I #2691 (comment) to choose one file first).

@harawata Oh, sorry, I misunderstood what you said before. Now I think it's a good idea. Should I close this PR and just do what you said?

@awxiaoxian2020
Copy link
Contributor Author

@hazendaz So should we restore the xdoc files? And what should I do to avoid the conflicts with them?

@hazendaz
Copy link
Member

hazendaz commented Feb 5, 2023 via email

@harawata
Copy link
Member

harawata commented Feb 5, 2023

@hazendaz ,

Main thing for me I think is being able to easily diff the two files but I haven't looked at it.

Diff between xdoc and markdown, you mean?
I don't think that is helpful as they are largely different.
Could you check that first, please?

@harawata
Copy link
Member

harawata commented Feb 5, 2023

Thank you for the update, @awxiaoxian2020 !

I noticed that you replaced the markdown tables with xdoc (html) tables.
There were align="left" on every cell, but other than that, it looked fine to me.
Was there any other reason for the change?

@hazendaz
Here is the diff between xdoc and markdown : https://gist.github.com/harawata/2477a55055c19021a2379ffb6b0c5334
It's pretty much useless, right?
Having both xdoc and markdown would make the review process more cumbersome, so there should be a good reason to keep the xdoc files.

@hazendaz
Copy link
Member

hazendaz commented Feb 5, 2023

ok yah that is hard to follow. main thing was if we lost any actual text in conversion or not.

@awxiaoxian2020
Copy link
Contributor Author

awxiaoxian2020 commented Feb 5, 2023 •
edited
Loading

Thank you for the update, @awxiaoxian2020 !

I noticed that you replaced the markdown tables with xdoc (html) tables. There were align="left" on every cell, but other than that, it looked fine to me. Was there any other reason for the change?

@hazendaz Here is the diff between xdoc and markdown : https://gist.github.com/harawata/2477a55055c19021a2379ffb6b0c5334 It's pretty much useless, right? Having both xdoc and markdown would make the review process more cumbersome, so there should be a good reason to keep the xdoc files.

The reason is that many tables have meta data such as <ul> <li> as you have pointed and <caption> for the title, which is not rendered by markdown table correctly. To say the least, there is no such problem. It is also uncomfortable to edit those metadata in the Markdown table. Although some tables do not have these metadata, since MyBatis documents rely on tables for many things, we cannot guarantee that complex metadata will not need to be added in the future, so I restore them to HTML tables.

harawata
harawata requested changes Feb 10, 2023
View reviewed changes
Copy link
Member

@harawata harawata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awxiaoxian2020 ,

It's getting better!
Three more changes and configuration.html is done, I believe. 🏃

Note that the metadata (title and author) should be added to the other pages.

src/site/markdown/configuration.md Show resolved Hide resolved
src/site/markdown/configuration.md Outdated Show resolved Hide resolved
src/site/markdown/configuration.md Show resolved Hide resolved
harawata
harawata requested changes Feb 11, 2023
View reviewed changes
Copy link
Member

@harawata harawata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update, @awxiaoxian2020 !

Let's move on to sqlmap-xml.md.
Please let me know if you have any question.

src/site/markdown/sqlmap-xml.md Outdated Show resolved Hide resolved
src/site/markdown/sqlmap-xml.md Outdated Show resolved Hide resolved
src/site/markdown/sqlmap-xml.md Outdated Show resolved Hide resolved
src/site/markdown/sqlmap-xml.md Show resolved Hide resolved
src/site/markdown/sqlmap-xml.md Show resolved Hide resolved
src/site/markdown/sqlmap-xml.md Outdated Show resolved Hide resolved
src/site/markdown/sqlmap-xml.md Outdated Show resolved Hide resolved
src/site/markdown/sqlmap-xml.md Show resolved Hide resolved
@awxiaoxian2020 awxiaoxian2020 marked this pull request as draft February 12, 2023 00:00
@awxiaoxian2020
update
d1654a6
@awxiaoxian2020
Merge branch 'master' into convert-md-en
ee0d063 
# Conflicts:
#	src/site/xdoc/configuration.xml
#	src/site/xdoc/logging.xml
#	src/site/xdoc/sqlmap-xml.xml
@awxiaoxian2020 awxiaoxian2020 requested a review from harawata March 23, 2023 07:15
@harawata
Copy link
Member

Thanks for the update, @awxiaoxian2020 !
Is this ready for a review?
I just wanted to be sure as it's still in draft status.

@awxiaoxian2020
Copy link
Contributor Author

@harawata I will check it on the weekend again. And I request a review just for my update. I don't know whther it's correct or not.

@awxiaoxian2020 awxiaoxian2020 marked this pull request as ready for review March 28, 2023 14:06
harawata
harawata reviewed Apr 2, 2023
View reviewed changes
pom.xml Outdated
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-site-plugin</artifactId>
<configuration>
<locales>en,es,ja,fr,zh_CN,ko</locales>
<siteDirectory>${project.build.directory}/site-src</siteDirectory>
Copy link
Member

@harawata harawata Apr 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hazendaz ,
Do those changes in pom.xml look OK to you?
It seems that the goal is to activate the variable substitution, but I'm not sure how or if it affects our already complicated site generation process.

If you are not sure, I'll revert the change.
Variable substitutions are nice, but not crucial and we can do it after the conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I copy it from https://github.com/mybatis/spring-boot-starter/blob/master/pom.xml

Copy link
Member

@harawata harawata Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but you also added some extra config like escapeString.

This review process is consuming my time a lot, so I would appreciate if you could do these extra stuff in separate PRs after we complete the xdoc -> markdown conversion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best I can tell, its just changing defaults for the site plugin which generally unless good reason we probably should not. Noting it was copied from spring boot is really meaningless to me. Spring promotes using their parent but fails to manage plugins properly and lets those all default to years old copies so IMO, they fail the maven smell test leaving the user to deal with it. Personally I don't like picking up their defaults and I'd bet its not even commented why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I love spring boot, just don't love that they leave maven users hanging by practically living off mavens defunct super pom and many developers don't understand that enough and wonder why builds are not stable as they could be. When you see this at scale it becomes very clearly evident since spring failed in that regard, its always better to use their second option in dependency management which makes the choices they did make mute.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then...not use their choices at all unless that came from maven or well documented why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hazendaz Sorry, I just want to use a value substitution instead of x.x.x. I didn't get your point. I just use mybatis-springboot-starter's workaround. Now, follow @harawata I revert it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update, @awxiaoxian2020 !
I'll review as soon as I find the time.

@hazendaz
Thank you for the comment!
I'm not sure, but could it be related to mybatis/spring#763 ?
Anyway, I'll forget about it for now :P

@awxiaoxian2020 awxiaoxian2020 requested a review from harawata April 3, 2023 10:28
@hazendaz
Copy link
Member

hazendaz commented Apr 4, 2023 via email

@hazendaz
Copy link
Member

hazendaz commented Apr 5, 2023 via email

@harawata harawata self-assigned this Apr 9, 2023
@harawata harawata added the documentation Indicates a changing on documentation(reference or javadoc) label Apr 9, 2023
@harawata harawata merged commit d7dcb48 into mybatis:master Apr 9, 2023
@harawata
Copy link
Member

harawata commented Apr 9, 2023

It's merged. Thank you, @awxiaoxian2020 !

Assuming you plan to continue the conversion, let us try a new way because this 'one language in one PR' strategy wasn't efficient very much.

  • Please convert only one file at a time (i.e. include only one file in one PR).
  • Please work in the following order.
    • es/configuration
    • ja/configuration
    • ko/configuration
    • zh/configuration

The goal is to shorten the time between open and merge for each PR which, I believe, minimizes the workload for both of us.
If there is a problem, we will re-adjust.

Oh, and please be sure to check my commit as most of the changes should be applied to the other languages when you work on them.

@awxiaoxian2020 awxiaoxian2020 deleted the convert-md-en branch April 9, 2023 08:42
harawata added a commit to epochcoder/mybatis-3 that referenced this pull request Jan 2, 2025
@harawata
Edited en doc a little bit and copied the doc changes to the other lo…
1562473 
…cales

Copying new doc is now much harder.
I knew this was going to happen... I shouldn't have budged 😣
mybatis#2784 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hazendaz hazendaz hazendaz left review comments

@harawata harawata harawata approved these changes

Assignees

@harawata harawata

Labels
documentation Indicates a changing on documentation(reference or javadoc)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@awxiaoxian2020 @coveralls @harawata @hazendaz