Asciidoctor: Be careful about bad callouts #602
Conversation
Stops the callout image copying code from attempting to copy the images for bad callouts and crashing.
| block.parent.context == :colist | ||
| id = block.attr('coids').scan(/CO(?:\d+)-(\d+)/) { | ||
| block.parent.context == :colist && | ||
| (coids = block.attr 'coids') |
There was a problem hiding this comment.
As I review more of this, my ability to read/remember ruby grows, but there are still some big holes.
So, correct me if I am wrong but, I think this code (as well as line 30 there) is:
- abusing
.attras an accessor to an instance variable (presumably to get around name requirements) - abusing that ruby allows assigning a value during a predicate check
- discarding the assigned symbol for the value, and
- boolean punning as an existence check over returning the assignment value as a side-effect
If I am right about any (or, yikes, all) of that, then yeah I feel I wouldn't be a good reviewer if I did not protest, because that's a whole lot of scary things.
There was a problem hiding this comment.
1. abusing `.attr` as an accessor to an instance variable (presumably to get around name requirements)
attr is a method on AbstractNode. I believe you are thinking of instance_variable_get. Or of attr_reader/attr_writer/attr syntax used to make instance members publicly accessible.
abusing that ruby allows assigning a value during a predicate check
It does indeed do that. I stayed away from it for a long time but the more of asciidoctor's code that I read the more this sort of behavior rubs off on me.
discarding the assigned symbol for the value, and
I use the result of the assignment on the next line I think.
There was a problem hiding this comment.
I use the result of the assignment on the next line I think.
Woops. I don't. I should be.
boolean punning as an existence check over returning the assignment value as a side-effect
It uses the "the result of an assignment is the value that was assigned" behavior and the ruby's default truthiness rules. I certainly didn't intend to throw out the result of the assignment.
There was a problem hiding this comment.
I've pushed a change that uses the attribute that I read.
I can certainly move the assignment if you'd like. I would have done it a few weeks ago but this is more in line with the way asciidoctor is written.
There was a problem hiding this comment.
@ddillinger, have a look at #604 - there I rewrote this if statement to be less "magical ruby".
There was a problem hiding this comment.
It solves a different problem but sets the style that I'd use to solve this one if you prefer that.
|
@ddillinger I've pushed an update to this uses the less "magical ruby" style. |
Stops the callout image copying code from attempting to copy the images for bad callouts and crashing.
Stops the callout image copying code from attempting to copy the images
for bad callouts and crashing.