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

Expose Section's sectnum property #1121

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

abelsromero
Copy link
Member

@abelsromero abelsromero commented Oct 9, 2022

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?
Exposes sectnum to users using the API for extensions, converters, etc.

This property is required to easily obtain the section numbering aligned with other Asciidoctor attributes. The alternative is reimplementing all the logic of level counting, secnumlevel, separators formating, etc.

How does it achieve that?

Adds 2 new methods:

  • String getSectnum()
  • String getSectnum(String delimiter)

to the Section interface, those simply call the Asciidoctor Ruby property (I assume property is the right name, it's a method).

Are there any alternative ways to implement this?

While working on a converter I could not find it. index returns 0, and there's a lot of reimplementation to do to obtain a similar result.

Right now I am using internals String sectnum = ((SectionImpl) node).getString("sectnum"), which is not ideal.

Are there any implications of this pull request? Anything a user must know?

  1. The property in Asciidoctor is well documented as public, I am assuming this is stable and can be used, but could be wrong. @mojavelinux can you confirm? 🙇
  2. I am keeping the default behauviour when sectnums is not set, that is, the delimiters without numbers is returned (see test should_return_invalid_sectnum_when_sectionNumbers_are_not_enabled). Is odd and I am tempted to add a check and return empty string, but maybe this should be fixed upstream? //cc @mojavelinux
  3. I am also not adding support to pass the append parameter, to keep things simple, and also adding a suffix is something that API users can just do just outside of the method.

Issue

n/a

Release notes

Please add a corresponding entry to the file CHANGELOG.adoc

@robertpanzer
Copy link
Member

Awesome, thank you!
It looks good to me.
I'll wait for feedback from @mojavelinux

@abelsromero Can you shed some light on the append parameter? I am not sure if I understood what you meant.

@abelsromero
Copy link
Member Author

abelsromero commented Oct 9, 2022

@abelsromero Can you shed some light on the append parameter? I am not sure if I understood what you meant.

If you look at the documentation in the Ruby implementation, the extra append param says...

the String to append at the end of the section number
             or Boolean to indicate the delimiter should not be
             appended to the final level

so, it allows the behaviours:

  • By default (null, not set, or true) it will appent the delimiter so we get something like "1.1." (see final dot.)
  • When false, won't append delimiter -> we get "1.1"
  • When set to a string, this is appended -> for example calling with "_", we get "1.1_"

I personally think (but it's subjective 😄) these options do not add much in terms of features. Any user that requires this level of fine tunning can do so outside of the method with normal String manipulations. And if in the future this is requested, we can always add it based on a real use case.

@abelsromero
Copy link
Member Author

Checking the errors in test-upstream, I am not sure the reason. Locallly works fine and the CI logs show the version installed being Version: 2.1.0.pre.alpha.0, not sure where the later SNAPSHOT in same ci execution comes from.

inconsistent module metadata found. Descriptor: rubygems:asciidoctor:2.1.0.pre.alpha.0-SNAPSHOT Errors: bad version: expected='2.1.0-alpha.0' found='2.1.0.pre.alpha.0-SNAPSHOT'

@robertpanzer
Copy link
Member

I guess this behavior might be related to this:
https://twitter.com/mojavelinux/status/1575764592675811328?s=20&t=St8K5u1adGt9-pJcR9veBQ

@mojavelinux
Copy link
Member

Yep, I need to switch core back to using dot to separate the prerelease component in the version because, once again, Bundler changed its rules and our workaround no longer works.

@abelsromero
Copy link
Member Author

Sorry to be insistent, but:

  • locally works fine (running the script or running line by line), at least in my Linux machine
  • logs show the gem is installed locally and the version

So , if I understand correctly the gem plugin is doing some extra remote check that is not happenign in my machine, and rubygems returns a different version number? If so, maybe we can disable that check.

@mojavelinux
Copy link
Member

mojavelinux commented Oct 10, 2022

The fix has been applied upstream, so now it should return the version number as entered in version.rb. See https://github.com/asciidoctor/asciidoctor/blob/main/lib/asciidoctor/version.rb. So if you're still seeing 2.1.0-alpha.0, then its being picked up from the cache (that version isn't actually released).

@mojavelinux
Copy link
Member

It appears like there are other problems:

inconsistent module metadata found. Descriptor: rubygems:asciidoctor:2.0.18.pre-SNAPSHOT Errors: bad version: expected='2.0.18.pre' found='2.0.18.pre-SNAPSHOT'

I don't know what the source of this check is, but it is clearly choking on this problem I alluded to in gem versioning. RubyGems doesn't allow the use of a hyphen, but semver doesn't allow a use of the hyphen after the third dot. It might be necessary to put some hacks somewhere in this build to deal with this incompatibility. The quick thing to do would be to change the version in version.rb to 2.0.18 after cloning.

@abelsromero
Copy link
Member Author

abelsromero commented Oct 10, 2022

I don't know what the source of this check is

gem-maven-plugin is doing it. The '-SNAPSHOT' is added to the pom descriptor's <version> but the artifact is installed under .m2/repository/rubygems/asciidoctor/2.1.0.alpha.0 (without it). That's why gradle mentions finding 2.0.18.pre-SNAPSHOT.

I found 3 solutions

  1. Append the -SNAPSHOP tail before installation, here.
  2. Append the -SNAPSHOP tail during test, here.
  3. Extracting the version from the installed pom and passing it during tests.

I'd say the third option is the secure one, unless we understand why the snapshot is added.

PS: tested bumping gem-maven-plugin, and it complains about some encoding issue in the pom I could not correct. Some misterious characters 🤷

@abelsromero abelsromero mentioned this pull request Oct 12, 2022
5 tasks
@abelsromero
Copy link
Member Author

Rebased the fix for upstream tests, we should be seeing 🟢 now

@robertpanzer
Copy link
Member

Thank you!

@robertpanzer robertpanzer merged commit d19336b into asciidoctor:main Oct 14, 2022
@abelsromero abelsromero deleted the expose-sections-sectnum branch April 21, 2023 08:25
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.

3 participants