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

Add a flag to toggle class-templating-syntax #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Bpolitycki
Copy link

This commit / PR adds a simple setting, which can be toggled by the configuration as part of the templating-model, to control if the old class based templating syntax (class='foo:bar') is interpreted as a call to a xquery function. This will allow to usage of colons inside classes (e.g. to use TailwindCSS)

@duncdrum
Copy link
Contributor

duncdrum commented Dec 5, 2023

The feature seems useful to me. Could you please add a test case and address the failing tests. We also need to document the feature in the templating article in documentation repo.

@Bpolitycki
Copy link
Author

The feature seems useful to me. Could you please add a test case and address the failing tests. We also need to document the feature in the templating article in documentation repo.

Great. Added tests and fixed the problem with the tests (I don't why, but I deleted the casting to booleans before).

So the place for the documentation would be over here: Templating Docs?

@duncdrum
Copy link
Contributor

duncdrum commented Dec 6, 2023

@Bpolitycki yes that is the spot. I ll try to take a look at the CI config later today. Your changes don't seem to have caused all these failures.

@Bpolitycki
Copy link
Author

@Bpolitycki yes that is the spot. I ll try to take a look at the CI config later today. Your changes don't seem to have caused all these failures.

Perfect – thanks. When running the tests locally, all tests are passing.

@Bpolitycki
Copy link
Author

I think there is a mistake in my approach (which was not covered by the tests).

https://github.com/Bpolitycki/exist-db-templating/blob/3fae54424b43676a1dbff6a5c04ba4cc15db6862/content/templates.xqm#L152C25-L152C25

Will stop the execution of templates on childs.

Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I like the feature.
Here are some things I would like to see:

  • I would prefer if we test different configurations as part of the unit tests
  • Please separate commits into changes, fixes and formatting where possible otherwise it is really hard to see what actually changed.
  • With which tool did you reformat the JS code and with what settings? We should be on the same page as to not constantly reformat those sources. Maybe best is to leave it out of this PR and first find common ground and set that as a separate build step.

package.json Outdated Show resolved Hide resolved
content/templates.xqm Outdated Show resolved Hide resolved
@@ -11,17 +11,17 @@ const { origin } = new URL(serverInfo.server);
const app = `${origin}/exist/apps/templating-test`;

const axiosInstance = axios.create({
baseURL: app
baseURL: app,
Copy link
Member

Choose a reason for hiding this comment

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

The comma is not necessary here.

Copy link
Member

Choose a reason for hiding this comment

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

It should not be part of this PR. See my comment on formatting above.

content/templates.xqm Outdated Show resolved Hide resolved
@@ -136,21 +138,26 @@ declare function templates:process($nodes as node()*, $model as map(*)) {
return
if ($dataAttr) then
templates:call($dataAttr, $node, $model)
else
else if (($model($templates:CONFIGURATION)($templates:CONFIG_USE_CLASS_SYNTAX), $templates:SEARCH_IN_CLASS)[1]) then
Copy link
Member

@line-o line-o Dec 6, 2023

Choose a reason for hiding this comment

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

$templates:CONFIG_USE_CLASS_SYNTAX should be set to true() in default configuration.
This will make the condition here much simpler as we can be sure this key is always set.

declare %private function templates:get-default-config($resolver as function(xs:string, xs:integer) as item()?) as map(*) {

Copy link
Member

Choose a reason for hiding this comment

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

else if ($model?($templates:CONFIGURATION)?($templates:CONFIG_USE_CLASS_SYNTAX)) then

Copy link
Author

Choose a reason for hiding this comment

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

Implemented in the suggested way. See 0a9b651

let $config := map {
$templates:CONFIG_APP_ROOT : $test:app-root,
$templates:CONFIG_STOP_ON_ERROR: true()
$templates:CONFIG_STOP_ON_ERROR: true(),
$templates:CONFIG_USE_CLASS_SYNTAX: xs:boolean(request:get-parameter('classLookup', $templates:SEARCH_IN_CLASS))
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to have different configurations to be checked as part of the unit tests. (see

declare variable $tt:config-no-filter := map {
and following)

@line-o
Copy link
Member

line-o commented Dec 6, 2023

@Bpolitycki If you need help or have further question, don't hesitate to ask here or on our community slack channel.

@Bpolitycki
Copy link
Author

Bpolitycki commented Dec 8, 2023

@Bpolitycki If you need help or have further question, don't hesitate to ask here or on our community slack channel.

@line-o Thanks for you feedback – I will have a look at the comments and try to fix the marked issued.

This commit adds a simple setting, which can be toggled by the configuration as part of the templating-model, to control if the old class based templating syntax (class='foo:bar') is interpreted as a call to a xquery function. This will allow to usage of colons inside classes (e.g. to use TailwindCSS)
This commit introduces a helper function (`templates:process-children`), which will ensure that all templates are called recursive. This is a fixes the bug introduces in the previous feature commit.
@Bpolitycki
Copy link
Author

@line-o – I'm really sorry, but it took quite a while to come back on this PR again.

I've worked on some of the requested changes – but a few are left open.

  1. I think the changes to the JS-formatting was done by VSCode (Prettier) – which formatter do you prefer? Let me know and I can fix the formatting.
  2. The test setup is a bit confusing for me. What's the purpose of splitting tests between JS and XQuery? Maybe you can give me an additional hint on how to implement the requested test scenario.

Thanks in advance!

@line-o
Copy link
Member

line-o commented Jan 22, 2024

@Bpolitycki Tests are split for two reasons:

  • XQuery: unit tests, allow to test different configurations
  • JS: integration tests

@line-o
Copy link
Member

line-o commented Jan 22, 2024

I, personally, do prefer standard. It is used in node-exist and xst.

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