asciidoctor / asciidoctorj

:coffee: Java bindings for Asciidoctor. Asciidoctor on the JVM!
http://asciidoctor.org
Apache License 2.0
627 stars 172 forks source link

Section.isNumbered() returns a boolean while it can also be the symbol :chapter in Asciidoctor #1122

Closed robertpanzer closed 1 year ago

robertpanzer commented 2 years ago

While looking through the changes in #1121 While looking through the code in Asciidoctor I stumbled over the fact that we have a property Section.numbered that is a boolean. However, looking at the code in Asciidoctor core, this property can seemingly have the values true, false, or the symbol :chapter: https://github.com/asciidoctor/asciidoctor/blob/4bab183b9f3fad538f86071b2106f3b8185d3832/lib/asciidoctor/parser.rb#L1599

Currently, AsciidoctorJ returns true in such case since the symbol seems to coerce to true as the following test indicates. This behavior is shown by this test:

    @Test
    public void isNumbered() {
        String content = "= Sample Document\n" +
                ":doctype: book\n" +
                ":sectnums: all\n" +
                "\n" +
                "[colophon]\n" +
                "== Test\n" +
                "\n" +
                "Test";

        Document document = asciidoctor.load(content, Options.builder()
                .build());

        List<StructuralNode> blocks = document.getBlocks();
        SectionImpl section = (SectionImpl) blocks.get(0);
        IRubyObject isNumbered = section.getRubyProperty("numbered");
        RubySymbol symbol = JRubyRuntimeContext.get(asciidoctor).newSymbol("chapter");
        assertSame(symbol, isNumbered);
        assertTrue(section.isNumbered());
    }

@mojavelinux Can you share some light on the meaning of the value :chapter? And should AsciidoctorJ also expose it?

mojavelinux commented 2 years ago

Normally, numbered is a boolean. However, the parser uses an internal value :chapter to distinguish when it should get the number of the chapter-number counter instead of the normal ordinal. See https://github.com/asciidoctor/asciidoctor/blob/main/lib/asciidoctor/abstract_block.rb#L407-L422 I did not really intend for this value to be public facing.

abelsromero commented 1 year ago

I did not really intend for this value to be public facing.

To be nice towards used we can mark as deprecated now and schedule for removal in Asciidoctorj v4.x.

abelsromero commented 1 year ago

Having fun with this, I was finally going to:

But I am not sure about the alternative for the deprecation Javadoc. For instance I was using it to format the title for maven-site integration here and right now the only thing I could think of to replace it, is checking if sectnum in the document

Section node = ...
return node.getDocument().getAttribute(Attributes.SECTION_NUMBERS) != null

@mojavelinux is this the method we want to encourage?

mojavelinux commented 1 year ago

I wasn't necessarily suggesting to remove the method, but to treat all non-null values as true in this API.

As you can see here, the converter only ever checks to see if the field is set: https://github.com/asciidoctor/asciidoctor/blob/main/lib/asciidoctor/converter/html5.rb#L387 The :chapter value is what is internal.