Skip to content

Template block macro (handlebars) #671

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

Merged
merged 14 commits into from
May 16, 2023
Merged

Conversation

osfameron
Copy link
Collaborator

See https://docs-staging.couchbase.com/home/contribute/extensions-template.html for a built demo of this.
(@mojavelinux I'll ping you the password for this separately)

This extension enables a [template] block which formats a specified .json file against a .hbs Handlebars template.

[template,example$some-data.json]
--
include::partial$templates/test.hbs[]

or, e.g:

{{#each items}}
* {{name}}
{{/each}}
--

See https://issues.couchbase.com/browse/DOC-10720 for a motivating example (metrics).

Other use-cases keep coming up:

etc.
`

This extension enables a `[template]` block which formats a
specified .json file against a .hbs Handlebars template.

```
[template,example$some-data.json]
--
include::partial$templates/test.hbs[]

or, e.g:

{{#each items}}
* {{name}}
{{/each}}
--
```

See https://issues.couchbase.com/browse/DOC-10720 for a motivating
example (metrics).

Other use-cases keep coming up:

 * OpenAPI to configuration schema generation,
 (currently done dynamically at e.g.
 https://docs.couchbase.com/sync-gateway/current/configuration-schema-database.html#_database )

 * Jira report -> release notes generation

etc.
    `
@osfameron osfameron requested review from leeming and a team April 24, 2023 12:37
@osfameron osfameron self-assigned this Apr 24, 2023
Copy link
Member

@malarky malarky left a comment

Choose a reason for hiding this comment

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

Hi @osfameron, as discussed, this looks like it definitely fulfils the primary requirement, which was to replace the current metrics reference tables with the JSON delivered from the builds. It also gives us a good starting point to be able to improve the docs with (for example) improved styling, version tags, deprecation notices, etc.

@@ -0,0 +1,44 @@
= Demo of template extension
Copy link
Contributor

Choose a reason for hiding this comment

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

(Minor) Niggle around the name "template", but that may just be the lack of familiarity with the tooling. Seems an overloaded term

Copy link
Contributor

@leeming leeming 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 as it seems to hit the functional ask. Some minor points, but not worth holding up progress for them, more FYI/opinion.


self.named('template')
self.positionalAttributes('resourceId')
self.onContext(['listing', 'open', 'paragraph'])
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend only using the open block (and maybe the paragraph). That's because the output becomes an open block. If it were used on a listing as is, you'd be changing the content model, which is not friendly to tooling. This could be resolved by detecting the original content module (by looking for cloaked-context), but it would be best to make that a separate enhancement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fine, I've previously used only open (for the "Markdown-ish" block) but I think I copied this from a doc/example in case it was best practice... will revert to just open 👍

return self.createOpenBlock(parent, output)
}
else {
return self.createOpenBlock(parent, `Data file ${attributes.resourceId} not resolved`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mojavelinux I'm confused on how to include Asciidoctor::Logging in JS rather than Ruby?

The second link for JS appears to be for choosing the overall Logger for the whole processing, as opposed to calling the logger from an extension.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yikes, I found this on the internets, but feels like there must be a simpler way?

  const Opal = global.Opal
  const loggerManager = Opal.module(null, 'Asciidoctor').LoggerManager
  const logger = loggerManager.getLogger()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I've pushed a commit with that in meantime, but look forward to pointers to improve!

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into it and the use of the Logging module in extensions is not yet well supported yet in Asciidoctor.js.

Here's the low-level JavaScript that would mix in the module:

self.$singleton_class().$include(global.Opal.Asciidoctor.Logging)

However, that won't give you access to getLogger() and createLogMessage() because this doesn't mix in Asciidoctor.js' porcelain interface.

It's probably best to use the Logging module directly:

const Logging = global.Opal.Asciidoctor.Logging

...

Logging.getLogger().error('message')

// or

Logging.getLogger().error(
  Logging.createLogMessage('message', { source_location: parent.getDocument().getReader().$cursor_at_mark() })
)

It's also possible to get the logger from the parent or reader:

parent.getLogger()
reader.getLogger()

That will give you the ability to log a basic message. However, in order to use createLogMessage to add context, you still need to use the Logging module.

@mojavelinux
Copy link
Contributor

In general, this seems sound to me. I left some comments in addition to the ones by Andrew. If those could be addressed, I'd give this a 👍.

Dan points out this syntax (though sounds like in future there
may be a way natively exposed via Asciidoctor.js)

We might also want to use Logging.createLogMessage, e.g.

  Logging.createLogMessage('message',
    { source_location: parent.getDocument().getReader().$cursor_at_mark() })
osfameron added a commit to osfameron/docs-server that referenced this pull request May 2, 2023
First cut - the data files are currently placeholders, and will be
updated by Dev once reviewed.

This build relies on feature in couchbase/docs-site#671
osfameron added 6 commits May 4, 2023 18:07
e.g. instead of attempting to do it in Handlebars
* Now linked and ready for POC in Mobile
(TODO remaining for additionalProperties, and will need iteration)

* markdown-block now uses a library (as the markdown in the new
OpenAPI spec benefits from this)
Though openapi spec defaults to yaml, yaml's pathological
flexibility with references means that, even with
`redocly bundle --defererenced` the definition of schemas
is spread across the file.

Outputting as JSON means that you get a flatter (and much
larger file). The advantage is that it's easier to fillet
(e.g. remove all the bits that aren't in the database schema)
to make it a smaller example file for docs-site.
@osfameron osfameron requested a review from sarahlwelton May 12, 2023 07:57
// we format it depending on its type (e.g. surrounding it in quotes etc.)

function format_type (node) {
if (node.type == 'object') return
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor?) My JS is rusty (some Typescript tho), but doesn't JS use triple === for boolean truthy, similar to PHP/Perl where strings are booleans, numbers and everything else

Copy link
Contributor

Choose a reason for hiding this comment

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

Both work, but you are strongly advised to use ===. The only time you should use == is when comparing against null (since that catches undefined too).

@osfameron
Copy link
Collaborator Author

The each-openapi functionality is used in couchbase/docs-sync-gateway#738 if more context is needed

Comment on lines +36 to +39
node.example ??
node.default ??
types[node.type]?.fallback ??
node.type ??
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why we're not trying to return the type of node first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logic is:

  • an example (e.g. "An interesting/illuminating value that could go here")
  • the default (e.g. 1)
  • the fallback (e.g. true for boolean values)
  • the plain type (e.g. string)
  • (unknown)

As most nodes have a type (in theory all of them but my JSON Schema parsing is a bit basic for additionalProperties handling) then that's in some sense the least interesting thing that we can say about them.

@osfameron osfameron merged commit a29545c into couchbase:master May 16, 2023
@osfameron osfameron deleted the hbs-templating branch May 16, 2023 07:11
osfameron added a commit to couchbase/docs-server that referenced this pull request Jun 23, 2023
* DOC-10720: Metrics Reference

First cut - the data files are currently placeholders, and will be
updated by Dev once reviewed.

This build relies on feature in couchbase/docs-site#671

* DOC-10720: Metrics Reference update

via Chris Malarky.
This updates 5 of the components, and reverts the rest to their previous
format.
simon-dew pushed a commit to simon-dew/docs-server that referenced this pull request Mar 7, 2024
* DOC-10720: Metrics Reference

First cut - the data files are currently placeholders, and will be
updated by Dev once reviewed.

This build relies on feature in couchbase/docs-site#671

* DOC-10720: Metrics Reference update

via Chris Malarky.
This updates 5 of the components, and reverts the rest to their previous
format.
simon-dew pushed a commit to couchbase/docs-server that referenced this pull request Mar 8, 2024
* DOC-10720: Metrics Reference

First cut - the data files are currently placeholders, and will be
updated by Dev once reviewed.

This build relies on feature in couchbase/docs-site#671

* DOC-10720: Metrics Reference update

via Chris Malarky.
This updates 5 of the components, and reverts the rest to their previous
format.
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.

5 participants