resolves #1719 add context and node_name accessor in the type definition #1718
Conversation
|
Hey! Could you please open an issue so we can discuss this change?
I don't recall removing this API 🤔
Could you please provide an example (in the issue) that rely on it? |
|
Yep, no problem. I'll work up an issue tomorrow. |
| AbstractNode.prototype.setNodeName = function (nodeName) { | ||
| this.node_name = nodeName | ||
| } |
There was a problem hiding this comment.
node_name is declared as an attribute reader: https://github.com/asciidoctor/asciidoctor/blob/7d856042fdd278f87d4c480180440e519fa1e97c/lib/asciidoctor/abstract_node.rb#L23-L24
@mojavelinux should we use attr_accessor on node_name?
There was a problem hiding this comment.
I'm going to warn you, we are really getting into some internal state here. The node_name variable has never been formalized and allowing it to be changed is really undefined. If anything, setting the context should set the node_name so it remains internal. But for an inline node, the node_name is "inline_" + the context.
There was a problem hiding this comment.
I'm reluctant to add it to the "porcelain" API. I would be in favour of adding two fields in the TypeScript definition node_name and context.
That way block.context = block.node_name = newName won't raise a warning.
There was a problem hiding this comment.
Mmm, yes. 🤔
It sounds like I'm skating very close to breaking stuff, now.
I have a vague memory of having to set the node name in the JavaScript API, but that wasn't needed for the Ruby API. I'll try a bit of test code to see if I can do this without fiddling with the node name.
Or perhaps I need to look at doing this a different way. Rather than alter the context, it would be safer to remove olist block and replace it with a colist built from scratch. (Would be more work though).
There was a problem hiding this comment.
I'm reluctant to add it to the "porcelain" API. I would be in favour of adding two fields in the TypeScript definition
node_nameandcontext.That way
block.context = block.node_name = newNamewon't raise a warning.
They would be picked up by the underlying Ruby API if I just add two fields in the TypeScript file?
There was a problem hiding this comment.
Yes, I think so.
As far as I recall, Opal is using un-prefixed fields for attr_reader.
There was a problem hiding this comment.
Yup, that seems to work 👍🏾
|
I'm not saying you can't set the node name, but rather that setting the
context should set the node name as the latter is internal.
…On Mon, Jan 1, 2024, 12:20 Ray Offiah ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/core/src/asciidoctor-core-api.js
<#1718 (comment)>
:
> +AbstractNode.prototype.setNodeName = function (nodeName) {
+ this.node_name = nodeName
+}
Mmm, yes. 🤔
It sounds like I'm skating very close to breaking stuff, now.
I have a vague memory of having to set the node name in the JavaScript
API, but that wasn't needed for the Ruby API. I'll try a bit of test code
to see if I can do this without fiddling with the node name.
Or perhaps I need to look at doing this a different way. Rather than alter
the context, it would be safer to remove olist block and replace it with a
colist built from scratch. (Would be more work though).
—
Reply to this email directly, view it on GitHub
<#1718 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATL5YM3X2SMXZY2BLDJULYMMEAJAVCNFSM6AAAAABBEHXEFWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJZHA2TIOBQGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@RayOffiah Looks good! Could you please add a test in: https://github.com/asciidoctor/asciidoctor.js/blob/main/packages/core/types/tests.ts. It should verify that declaring |
|
I still don't think node_name should be exposed. Instead, setContext should set context and node_name. In Ruby, context is a symbol and node_name is a string, but in JavaScript there are no symbols (of the Ruby variety), so these two fields can just be synced. There's no reason that context and node_name should be different. This was purely an internal optimization to split them. |
Okay, will do. 👍🏾 |
context and node_name now exposed as properties. add setContext function to API
|
All good! |
ggrossetie
changed the title
|
I've made my case ;) |
Hi there 👋🏾
In previous versions of the API, it was possible to change the context and name of a block/node by reassigning it in the source code.
I noticed that the ability to do this has been removed from the API.
It isn't a function that is going to get used a lot, but some code I wrote a while back does rely on it.
This PR will restore the functionality, but uses proper
setfunctions to change the values.