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

feat(parser/renderer): add user macro feature #347

Merged
merged 2 commits into from
May 15, 2019

Conversation

odknt
Copy link
Contributor

@odknt odknt commented May 12, 2019

fixes #334

  • add parse rules for user macro to parser.
  • add renderer.DefineMacro() that returns renderer.Option to renderer.
  • user can define a rendering function for macro by call renderer.DefineMacro() and pass to renderer.Wrap().

ex.

func helloMacro(cm types.UserMacro) ([]byte, error) {
    // render content
}

...
    renderer.Wrap(context.Background(), doc, renderer.DefineMacro("hello", helloMacro))
...

@codecov
Copy link

codecov bot commented May 12, 2019

Codecov Report

Merging #347 into master will increase coverage by 0.06%.
The diff coverage is 92.68%.

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   86.37%   86.43%   +0.06%     
==========================================
  Files          46       47       +1     
  Lines        4176     4217      +41     
==========================================
+ Hits         3607     3645      +38     
- Misses        425      427       +2     
- Partials      144      145       +1

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.

This looks great, @odknt 🙌
I have not seen how those macros can be registered and associated with a template when running from the CLI (using a config file for example). Is it something you plan to add in this PR? (we could also discuss about it in a subsequent PR if you prefer.

pkg/renderer/html5/user_macro_test.go Outdated Show resolved Hide resolved
@odknt
Copy link
Contributor Author

odknt commented May 13, 2019

I have not seen how those macros can be registered and associated with a template when running from the CLI (using a config file for example).

Sorry, I didn't read latest comment of the issue carefully.
I did refactor to be use Go template.

pkg/renderer/context.go Outdated Show resolved Hide resolved
@xcoulon
Copy link
Member

xcoulon commented May 13, 2019

how do you associate a template with a macro if you have multiple files in the _macros sub-directory?

ah, I see: the name of the file is the directly associated with the macro, isn't it? In this case, could we process the filename to remove its extension, so users can name their template files to something like hello.tmpl or hello.txt?
Also, in the future, we may need to be able to configure the path to the directory which contains the templates. I'm not sure if pkg/renderer/html5/_macros/ is suitable for users.

@xcoulon xcoulon mentioned this pull request May 13, 2019
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.

Requesting change with regards to how to proceed with undefined macros: do not fail, but just output the raw text as-is.

pkg/types/grammar_types.go Outdated Show resolved Hide resolved
pkg/renderer/html5/user_macro_test.go Show resolved Hide resolved
pkg/renderer/html5/user_macro_test.go Outdated Show resolved Hide resolved
pkg/renderer/html5/renderer_test.go Outdated Show resolved Hide resolved
pkg/renderer/html5/renderer_test.go Outdated Show resolved Hide resolved
@odknt
Copy link
Contributor Author

odknt commented May 14, 2019

Test failed on Windows due to LF replaced with CRLF in template file.
I thinking of remove findMacroTemplate() and replace template file with string.

Please tell me your thoughts.

@xcoulon
Copy link
Member

xcoulon commented May 14, 2019

Test failed on Windows due to LF replaced with CRLF in template file.
I thinking of remove findMacroTemplate() and replace template file with string.

Please tell me your thoughts.

@odknt yes, it's safe to move the content of pkg/renderer/html5/_macros/hello into a string variable at this stage. We will have to deal with files in #349

One thing though: I had a similar problem a while ago, and I was able to fix it with the following git config: core.autocrlf: input. Maybe this will help?

@odknt odknt force-pushed the Issue334_user_macro branch from a06be07 to f775133 Compare May 14, 2019 13:50
@odknt
Copy link
Contributor Author

odknt commented May 14, 2019

I squashed commits for cleanup.

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.

Great work @odknt 🙌
One last thing: could you mention this feature (along with the code example in the PR description) in the README.adoc, too? (We should probably have a separate file to list the features in the future) Then it'll be all good and I'll merge your PR.

@odknt
Copy link
Contributor Author

odknt commented May 15, 2019

I added a section about the feature to README.adoc, but I'm no good at English so my sentences may be terrible.

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.

Thanks a lot for your contribution, @odknt! 🙌🎉

@xcoulon xcoulon merged commit 96b01cf into bytesparadise:master May 15, 2019
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.

Custom tags definition
3 participants