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

[github.io] Convert acle rst to md #134

Conversation

ValeriaDaneva
Copy link
Contributor

No description provided.

main/acle.md Outdated Show resolved Hide resolved
main/acle.md Outdated Show resolved Hide resolved
main/acle.md Outdated Show resolved Hide resolved
@ValeriaDaneva ValeriaDaneva force-pushed the github.io-convert-acle-rst-to-md branch from 277838d to ba1c3b6 Compare December 6, 2021 16:33
@ValeriaDaneva ValeriaDaneva force-pushed the github.io-convert-acle-rst-to-md branch from ba1c3b6 to ed4caf4 Compare December 7, 2021 16:55
Copy link
Contributor

@fpetrogalli fpetrogalli left a comment

Choose a reason for hiding this comment

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

Hi @ValeriaDaneva - unfortunately this PR needs some re-work. There are too many changes in the rst-to-md content change (the second commit) that are unrelated to the syntax change. The huge amount of changes that are not really needed makes it quite difficult to review.

Most of the unrelated changes seems to be coming from situations like the following:

Screenshot 2021-12-07 at 17 46 54

In cases like this, a simple change like replacing the syntax for fixed font text, which would have resulted in one line change in the patch, have become a 4 or 5 line change for a simple rearrangement of the text in the lines. This is (probably) due to the script that you have used to remove the wrong syntax generated by pandoc?

Please go through the rst-to-md content change again and minimize the amount of changes that need to be done for these kind of things. This will make it easier to review, and will also increase the guarantee that what we have is just a syntax change and not a content change. Right now I would have to go through all the paragraphs to make sure that the text is correct to be able to tell if we can merge the patch. Too risky and error prone.

There are also paragraphs that show as changed although there are no real changes in it, just a different length of the line. We better keep those paragraphs to the original version.

I suspect that these useless changes have been introduced by the automatic line justification introduced by pandoc in the conversion process...

main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
@ValeriaDaneva ValeriaDaneva force-pushed the github.io-convert-acle-rst-to-md branch 2 times, most recently from 24b4a38 to dc1742e Compare December 10, 2021 18:41
Copy link
Contributor

@fpetrogalli fpetrogalli left a comment

Choose a reason for hiding this comment

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

Hi @ValeriaDaneva - thank you for simplifying the changes, this made it much easier to review.

Now, I have left many comments, but most of them are duplicate instances of two issues.

  1. The syntax for wrapping C code is not c ... , but triple , followed by c. However, we don't want to introduce C syntax highlighting in this change, just wrap the code with triple
  2. Some code (mostly intrinsic definitions) was originally written in rst with the syntax that uses ::. This syntx renders the text as a separate paragraph, not inline. You have replaced this rst syntax with single wrapping, which renders it inline. You have to change these instances by wrapping them with a triple.

There are many instances of both 1. and 2., I haven't marked them all.

Thank you for your patience, we are getting there!

Francesco

main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
main/acle.rst Outdated Show resolved Hide resolved
@ValeriaDaneva ValeriaDaneva force-pushed the github.io-convert-acle-rst-to-md branch from 0a8d796 to e1521f3 Compare December 13, 2021 15:17
Copy link
Contributor

@fpetrogalli fpetrogalli 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 @ValeriaDaneva ! LGTM

@fpetrogalli fpetrogalli merged commit 13babf9 into ARM-software:next-release-github.io-transition Dec 13, 2021
@ValeriaDaneva ValeriaDaneva deleted the github.io-convert-acle-rst-to-md branch December 13, 2021 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants