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

feature(types/parser/renderer): Support for counters #715

Merged
merged 4 commits into from
Jul 12, 2020

Conversation

gdamore
Copy link
Collaborator

@gdamore gdamore commented Jul 11, 2020

This adds support for {counter:name} and {counter2:name}
as attribute substitutions that reference the named counters.

The existing table, image, etc. counters are not converted to use
these yet, but it's likely that doing so will help once attribute
substitution is supported.

Note that when that work is done, care must be taken to ensure that
the counter is incremented when expanded to it's literal value, not
just expanded from the built-in value.

Fixes #714

@codecov
Copy link

codecov bot commented Jul 11, 2020

Codecov Report

Merging #715 into master will decrease coverage by 0.05%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
- Coverage   84.89%   84.83%   -0.06%     
==========================================
  Files          75       75              
  Lines        5057     5105      +48     
==========================================
+ Hits         4293     4331      +38     
- Misses        480      489       +9     
- Partials      284      285       +1     

@gdamore gdamore force-pushed the counters branch 4 times, most recently from 660181b to 077ae41 Compare July 12, 2020 05:12
Copy link
Member

@xcoulon xcoulon left a comment

Choose a reason for hiding this comment

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

looks good overall. Just a couple of questions/suggestions

pkg/parser/parser.peg Outdated Show resolved Hide resolved
Copy link
Member

@xcoulon xcoulon left a comment

Choose a reason for hiding this comment

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

After further reading the Asciidoctor docs about counters, I believe that counter2 does not behave as expected in this PR :(

@gdamore
Copy link
Collaborator Author

gdamore commented Jul 12, 2020

Actually you've picked up the stray debug message. Stay tuned for an update. I will probably squash this.

This adds support for {counter:name} and {counter2:name}
as attribute substitutions that reference the named counters.

The existing table, image, etc. counters are not converted to use
these yet, but it's likely that doing so will help once attribute
substitution is supported.

Note that when that work is done, care must be taken to ensure that
the counter is incremented when expanded to it's literal value, not
just expanded from the built-in value.

This includes support for setting the start including alpha counters
(English letters only, same as asciidoctor).

Fixes bytesparadise#714
@gdamore
Copy link
Collaborator Author

gdamore commented Jul 12, 2020

Please re-review. I believe I've fixed the semantics of counter2.

I've also added support for using the set syntax with counter2 (to set the starting index without displaying) in precisely the same manner that asciidoctor does. (Tested this against asciidoctor as well.)

@xcoulon
Copy link
Member

xcoulon commented Jul 12, 2020

Please re-review. I believe I've fixed the semantics of counter2.

I've also added support for using the set syntax with counter2 (to set the starting index without displaying) in precisely the same manner that asciidoctor does. (Tested this against asciidoctor as well.)

cool, thanks for fixing the PR. I'll merge it once all builds passed.

@gdamore
Copy link
Collaborator Author

gdamore commented Jul 12, 2020

Want me to rebase this?

@xcoulon
Copy link
Member

xcoulon commented Jul 12, 2020

Want me to rebase this?

I've already rebased the PR and I can squash the commits when merging. Unless you want to do it yourself?

@gdamore
Copy link
Collaborator Author

gdamore commented Jul 12, 2020

Nah, if you're good handling it, that's 100% fine with me. ;-)

@xcoulon xcoulon merged commit b9e82cd into bytesparadise:master Jul 12, 2020
@xcoulon
Copy link
Member

xcoulon commented Jul 12, 2020

PR merged. Thanks @gdamore! 🙌

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.

Want attribute substitutions for {counter:name} and {counter2:name}
2 participants