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

Document breaking changes to processors in migration guide #1210

Merged
merged 3 commits into from
May 31, 2023

Conversation

robertpanzer
Copy link
Member

Thank you for opening a pull request and contributing to AsciidoctorJ!

Please take a bit of time giving some details about your pull request:

Kind of change

  • Bug fix
  • New non-breaking feature
  • New breaking feature
  • Documentation update
  • Build improvement

Description

What is the goal of this pull request?

This PR documents the breaking changes to the processor API in the migration guide from 2.5.x to 3.x.x.

(Please note that I used ChatGPT to rephrase parts of that text to better convey the necessary steps, I often find myself writing texts that native speakers are not able to follow (guess whose fault that is 😜))

@robertpanzer robertpanzer requested a review from abelsromero May 29, 2023 15:16
Copy link
Member

@abelsromero abelsromero left a comment

Choose a reason for hiding this comment

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

I am obsessive to a fault with the use of formatting (verbatim, italics, etc.). I marked a few words that I think should be highlighted, but the text itself is perfectly clear without it too.
I leave it to your criteria.

Moreover, there was a problem with the first parameter of the process method in InlineMacroProcessors.
Previously, it incorrectly expected a PhraseNode as the first parameter.

To resolve this issue, AsciidoctorJ 3.x.x has rectified the first parameter of InlineMacroProcessor::process to be a StructuralNode.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Make sure to modify your implementation to return a StructuralNode as required.

Moreover, there was a problem with the first parameter of the process method in InlineMacroProcessors.
Previously, it incorrectly expected a PhraseNode as the first parameter.
Copy link
Member

Choose a reason for hiding this comment

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

I like to make it clear when I am pointing to a class or method using backticks (`).

Suggested change
Previously, it incorrectly expected a PhraseNode as the first parameter.
Previously, it incorrectly expected a `PhraseNode` as the first parameter.

@@ -0,0 +1,43 @@
== Update Preprocessors

In earlier versions of AsciidoctorJ (up to 2.5.x), Preprocessors were provided with a Reader object that they had to modify directly.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In earlier versions of AsciidoctorJ (up to 2.5.x), Preprocessors were provided with a Reader object that they had to modify directly.
In earlier versions of AsciidoctorJ (up to 2.5.x), Preprocessors were provided with a `Reader` object that they had to modify directly.

}
----

You may find it helpful to review the methods Processor::newReader and PreprocessorReader::read to simplify your code when creating a new Reader and reading the content of the existing Reader.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You may find it helpful to review the methods Processor::newReader and PreprocessorReader::read to simplify your code when creating a new Reader and reading the content of the existing Reader.
You may find it helpful to review the methods `Processor::newReader` and `PreprocessorReader::read` to simplify your code when creating a new Reader and reading the content of the existing Reader.

@robertpanzer robertpanzer force-pushed the extension-migration-guide branch from 9341dd7 to dfb44e3 Compare May 31, 2023 15:35
@robertpanzer robertpanzer merged commit 64c1977 into asciidoctor:main May 31, 2023
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