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

Parse "jbake-" prefixed asciidoc headers like normal metadata #580

Merged
merged 3 commits into from Jan 19, 2019

Conversation

Projects
None yet
4 participants
@matthiasblaesing
Copy link
Contributor

matthiasblaesing commented Jan 6, 2019

JBake explicitly supports adding complex metadata in the form of string
encoded JSON objects. This support was missing from metadata sourced
from native asciidoc headers. The support is only added for explicit
"jbake-" headers, to not interfere with normal asciidoc operation.

Parse "jbake-" prefixed asciidoc headers like normal metadata
JBake explicitly supports adding complex metadata in the form of string
encoded JSON objects. This support was missing from metadata sourced
from native asciidoc headers. The support is only added for explicit
"jbake-" headers, to not interfere with normal asciidoc operation.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 6, 2019

Coverage Status

Coverage increased (+0.007%) to 80.787% when pulling 20c054c on matthiasblaesing:asciidoc_inline_header into 5261a32 on jbake-org:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 6, 2019

Coverage Status

Coverage decreased (-0.1%) to 80.677% when pulling 51aaa01 on matthiasblaesing:asciidoc_inline_header into 5261a32 on jbake-org:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 6, 2019

Coverage Status

Coverage decreased (-0.1%) to 80.677% when pulling 51aaa01 on matthiasblaesing:asciidoc_inline_header into 5261a32 on jbake-org:master.

@ancho

This comment has been minimized.

Copy link
Member

ancho commented Jan 10, 2019

Interesting. Thank you. I schedule some time for a review. Maybe end of the weekend.

@ancho ancho self-requested a review Jan 10, 2019

@ancho
Copy link
Member

ancho left a comment

Thanks for catching this. I added a few comments I would like to discuss.

content.put(key, JSONValue.parse(value));
} else {
content.put(key, value);
void processHeader(String inputKey, String inputValue, Map<String, Object> content) {

This comment has been minimized.

@ancho

ancho Jan 15, 2019

Member

I'm searching for a better name here. How about processHeaderValue

This comment has been minimized.

@matthiasblaesing

matthiasblaesing Jan 16, 2019

Author Contributor

I'm ok with changing the name, I used processHeader because this is key/value, an alternative would be storeHeaderValue, but I have no strong feeling into any direction.

This comment has been minimized.

@ancho

ancho Jan 17, 2019

Member

Thats even a better name. That alignes with my attempt to build a document and template model abstraction. Which could be a new home for this method later. See #532

if(value instanceof String) {
processHeader(pKey, (String) value, documentModel);
} else {
documentModel.put(pKey, value);

This comment has been minimized.

@ancho

ancho Jan 15, 2019

Member

We could always call processHeader. As the last case in that method adds the value for the given key to the documentModel Map. See line 284 in MarkupEngine.

This comment has been minimized.

@matthiasblaesing

matthiasblaesing Jan 16, 2019

Author Contributor

I decided to special case the "String" case, as I don't want to interfere with the asciidoc engine. As NULL could be generated, we'd still need a guard as in

if(value != null) {
    processHeader(pKey, value.toString(), documentModel);
}

I have zero experience with asciidocj, so can't predict if other types, than string are possible.

This comment has been minimized.

@ancho

ancho Jan 17, 2019

Member

Yes you are right. asciidoctorj takes a Map<Sting, Object> for attributes. So it would be worth a try. On the other hand we could postpone it for a later cleanup session.

@matthiasblaesing

This comment has been minimized.

Copy link
Contributor Author

matthiasblaesing commented Jan 17, 2019

I pushed two updates, that try to address the comments. I kept them separate to make review easier, for integration I would squash them.

@jonbullock jonbullock added this to the v2.6.4 milestone Jan 18, 2019

@ancho ancho merged commit 20c054c into jbake-org:master Jan 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonbullock

This comment has been minimized.

Copy link
Member

jonbullock commented Jan 22, 2019

Thanks for the contribution @matthiasblaesing 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment