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

Implemented templates resolution for functions #24

Merged
merged 3 commits into from
Aug 17, 2021

Conversation

vemonet
Copy link
Contributor

@vemonet vemonet commented Aug 12, 2021

templates are when the user provides constant strings + reference variables to a function parameter, e.g. This person is: {FirstName} {LastName}

It was not resolving before (only constant and reference were implemented)

All tests are passing

@vemonet vemonet mentioned this pull request Aug 12, 2021
@ThibaultGerrier
Copy link
Collaborator

Looks good!

This won't work with getting multiple values from the template:
e.g.

{ 
  "mult": ["foo", "bar"]
}

template: "{mult.*}" -> "foo,bar" instead of ["foo", "bar"]
or
template: "val: {mult.*}" -> "vals: foo,bar" instead of ["val: foo", "val: bar"]
or
template: "{mult.*}-{mult.*}" -> "foo,bar-foo-bar" instead of ["foo-foo", "foo-bar", "bar-foo", "bar-bar"] (doing all permutations is how templates work with the Java RML implementation)

If we'd want to support multiple values we could reuse calculateTemplate from

const calculateTemplate = (Parser, index, template, prefixes, termType, options) => {
. It would need a bit of refactoring though. (also looking at the code, termtype probably does not work with functions, but that's another issue..)

But since this is such a niche usecase and I'm not sure if it is supposed to work like that with functions, I'd be ok with this single string behavior.

If you could just add a var/let/const to 760f56e#diff-c739baa938dd5fea06c035a19eb7900313b820973dd508f0a8a7a644795b7cbaR32

@vemonet
Copy link
Contributor Author

vemonet commented Aug 17, 2021

Indeed using calculateTemplate would make sense I wish I noticed it earlier! I fixed the missing const in the current implementation

@ThibaultGerrier ThibaultGerrier merged commit 8ddbbab into semantifyit:master Aug 17, 2021
@ThibaultGerrier
Copy link
Collaborator

Great thanks!
Published with v1.11.1

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.

2 participants