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

The newline option in the template module does not behave according to documentation #466

Closed
RiskoZoSlovenska opened this issue Feb 14, 2024 · 4 comments · Fixed by #467
Closed
Assignees

Comments

@RiskoZoSlovenska
Copy link
Contributor

The documentation for template.compile states that

newline: string to replace newline characters, default is nil (not replacing newlines).

However, as can be seen in the code, a truthy value simply replaces all newlines with an empty string, and a falsy value does not:

    if newline then
        r = format("%q", strgsub(strsub(chunk,s),"\n",""))
    else

In other words, passing in a string different from the empty string does not actually replace the newlines with that string, contrary to what the documentation says.

I'd gladly PR a fix, but I'm not sure what the intended behaviour was actually meant to be; is it the documentation or the code that needs to be changed?

@alerque
Copy link
Member

alerque commented Feb 14, 2024

Fixing the docs should be pretty straight forward. Changing the behavior might be a bit trickier because depending on the change it might be considered breaking. What is the really useful use case here? And is there a way to make it do what you want without possible breaking existing code?

@RiskoZoSlovenska
Copy link
Contributor Author

I don't need different behaviour myself (I can't think of a use-case where you'd want to replace newlines with something else), I just noticed the discrepancy while trying to implement an unrelated enhancement. Simply updating the docs would be fine. Renaming the option to something like strip_newlines (and supporting the old name too for compat) would also be nice, but not necessary.

@alerque
Copy link
Member

alerque commented Feb 14, 2024

If you don't have a use case for something else, lets just fix the docs to document what we do. That will be a docs fix not a potentially breaking change.

Care to submit a PR?

@RiskoZoSlovenska
Copy link
Contributor Author

Sure

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

Successfully merging a pull request may close this issue.

2 participants