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

Moved context functions to ContentNode #1250

Merged
merged 1 commit into from
Dec 16, 2023
Merged

Moved context functions to ContentNode #1250

merged 1 commit into from
Dec 16, 2023

Conversation

RayOffiah
Copy link
Contributor

@RayOffiah RayOffiah commented Dec 9, 2023 •
edited
Loading

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?
Adding a function to set the Context of a node.

How does it achieve that?
Added a setContext function to the ContentNode

Are there any alternative ways to implement this?
Possibly.

Are there any implications of this pull request? Anything a user must know?
Not that I know of.

Issue

If this PR fixes an open issue, please add a line of the form:

Fixes #Issue

Release notes

Please add a corresponding entry to the file CHANGELOG.adoc

@abelsromero
Copy link
Member

I imagine this this related to this conversation https://asciidoctor.zulipchat.com/#narrow/stream/279644-users.2Fasciidoctorj/topic/Setting.20block.20context.20in.20AsciidoctorJ/near/406515443.

I have 2 comments:

  1. At first glance, it's weird to move the methods to StructuralNode because it is under ContentNode. If we consider the property to be "writable", why not directly in ContentNode?

  2. It'd be great to better understand the use case if you could add a test with the use case you mentioned in Zulip? I do wonder if a new node could just be built some other way?

@RayOffiah RayOffiah changed the title Moved context functions to StructuralNode Moved context functions to ContentNode Dec 10, 2023
@RayOffiah RayOffiah marked this pull request as ready for review December 10, 2023 09:24
robertpanzer
robertpanzer reviewed Dec 10, 2023
View reviewed changes
Copy link
Member

@robertpanzer robertpanzer left a comment

Choose a reason for hiding this comment

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

Would you mind adding a test case to make sure that the method really works?

asciidoctorj-core/src/main/java/org/asciidoctor/jruby/ast/impl/ContentNodeImpl.java Outdated Show resolved Hide resolved
robertpanzer
robertpanzer reviewed Dec 16, 2023
View reviewed changes
CHANGELOG.adoc Outdated Show resolved Hide resolved
@RayOffiah
Added unit test for changing the context: ContextChangeTest.java
60c1642 
Reformatting the setContext function.
Added entry to CHANGELOG.adoc
robertpanzer
robertpanzer approved these changes Dec 16, 2023
View reviewed changes
Copy link
Member

@robertpanzer robertpanzer left a comment

Choose a reason for hiding this comment

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

LGTM

@robertpanzer robertpanzer merged commit 4315499 into asciidoctor:main Dec 16, 2023
12 checks passed
@robertpanzer
Copy link
Member

Thanks!
Feel free to also open a PR against the v2.5.x branch if you want to have this in a minor release before 3.0.0 is released.

@RayOffiah RayOffiah deleted the add-setContext-to-API branch December 18, 2023 07:39
@RayOffiah
Copy link
Contributor Author

RayOffiah commented Dec 18, 2023 via email

RayOffiah added a commit to RayOffiah/asciidoctorj that referenced this pull request Dec 18, 2023
@RayOffiah
Fixing unit test
a1b2b59 
Added unit test for changing the context: ContextChangeTest.java (asciidoctor#1250)
Reformatting the setContext function.
Added entry to CHANGELOG.adoc
(cherry picked from commit 4315499)
@RayOffiah RayOffiah mentioned this pull request Dec 18, 2023
Merged
5 tasks
robertpanzer pushed a commit that referenced this pull request Dec 20, 2023
@RayOffiah
Fixing unit test (#1253)
38fdb22 
Added unit test for changing the context: ContextChangeTest.java (#1250)
Reformatting the setContext function.
Added entry to CHANGELOG.adoc
(cherry picked from commit 4315499)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertpanzer robertpanzer robertpanzer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RayOffiah @abelsromero @robertpanzer