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

gcode_macro: Enable jinja2.ext.do extension #5149

Closed
wants to merge 2 commits into from

Conversation

r3Fuze
Copy link

@r3Fuze r3Fuze commented Jan 13, 2022

This adds a 'do' tag that works like a normal expression but ignores the return value.
Useful for avoiding running the output of an expression as a command when it is the only thing on a line.

Example scenarios:

Without extension enabled:

[gcode_macro NONE_TEST]
gcode:
  { [1,2].append(3) }

Console output: Unknown command:"NONE"

[gcode_macro NONE_TEST]
gcode:
  {% do [1,2].append(3) %}

Klippy error: TemplateSyntaxError: Encountered unknown tag 'do'.


With extension enabled:

[gcode_macro NONE_TEST]
gcode:
  {% do [1,2].append(3) %}

No console output

I don't know how needed this is since I couldn't find anyone else complaining about Unknown command:"NONE" in the console, and there are workarounds to avoid it ({% set _ = [1,2].append(3) %}) but it is a very simple feature so it shouldn't hurt anything.

Signed-off-by: Morten Lindhardt [email protected]

This adds a 'do' tag that works like a normal expression but ignores the return value.
Useful for avoiding running the output of an expression as a command.

Signed-off-by: Morten Lindhardt <[email protected]>
Signed-off-by: Morten Lindhardt [email protected]
@zellneralex
Copy link
Contributor

I use the workaround normally

@r3Fuze
Copy link
Author

r3Fuze commented Jan 13, 2022

@r3Fuze as a note there is a workaround for that

{% set dummy = [1,2].append(3) %}

A real function Klipper example would be e.g. zellneralex/klipper_config@master/flexplate.cfg#L127

That's the workaround I'm using for now too. Funnily enough, I was working on a flexplate system very similar to yours when I ran into this 😅

@zellneralex
Copy link
Contributor

zellneralex commented Jan 19, 2022

Maybe that helps, here is a real live example (the complete macros can be found https://github.com/zellneralex/klipper_config/blob/master/flexplate.cfg

This is how we need to solve it currently:

[gcode_macro LIST_PLATES]
description: List all flexplates
gcode:
  {% if not printer.save_variables.variables.plates %}
    {action_respond_info("FLEXPLATE: No Plate Array defined. ABORDED")}
  {% else %}
    {% set plates = printer.save_variables.variables.plates %}
    {% set out = ["FLEXPLATE: Defined Plates"] %}
    {% for elem in range(plates.array|length) %}
      {% set _dummy = out.append("INDEX: %d -> %s -> offset: %.3fmm" % 
                      (elem, plates.array[elem].name, plates.array[elem].offset)) %}
    {% endfor %}
    {% set  _dummy = out.append("\n Active Plate: %s" % plates.array[plates.index].name) %}
    {action_respond_info(out|join("\n"))}
  {% endif %}

It works with the work around mentioned already.

After the PR would be merged it will look like:

[gcode_macro LIST_PLATES]
description: List all flexplates
gcode:
  {% if not printer.save_variables.variables.plates %}
    {action_respond_info("FLEXPLATE: No Plate Array defined. ABORDED")}
  {% else %}
    {% set plates = printer.save_variables.variables.plates %}
    {% set out = ["FLEXPLATE: Defined Plates"] %}
    {% for elem in range(plates.array|length) %}
      {% do out.append("INDEX: %d -> %s -> offset: %.3fmm" % 
                      (elem, plates.array[elem].name, plates.array[elem].offset)) %}
    {% endfor %}
    {% do out.append("\n Active Plate: %s" % plates.array[plates.index].name) %}
    {action_respond_info(out|join("\n"))}
  {% endif %}

So the change looks small but believe me as a person answers a lot of question about macros and the examples I provide I would appreciate this change since it makes it clearer what happens in that lines.

BTW testet in a python2 and python3 env

@jschuh
Copy link
Contributor

jschuh commented Feb 3, 2022

FWIW, I just found myself in need of this today, so it definitely seems like a good idea (and seems very low risk).

Some minor suggestions: Commit messages should be wrapped at 75 characters or less, and if this were my PR I'd just squash it into a single commit while fixing the log entry and then do a force push. (That just makes it easier to land.)

@KevinOConnor
Copy link
Collaborator

KevinOConnor commented Mar 4, 2022

Thanks. I understand this proposal; however I don't currently agree with it.

I agree that the Jinja2 syntax is cryptic and can be hard to use. Using Jinja2 was always a compromise - as crytpic as it is, it was better than creating a completely new "roll your own" cryptic programming language.

So, I agree that {% set dummy = func() %} is cryptic. However, I also view {% do func() %} to be nearly as cryptic. And, there are some disadvantages of the latter: 1- we'd have to document that the extension is active as it wont be immediately obvious to users that they can use do or what it does, and 2 - after making this change some users will prefer "ugly syntax 1" over "ugly syntax 2" and we'll ultimately have more permutations of "ugly".

So, my preference at this time is to try to keep the Jinja2 usage as simple as possible. Yes, it has issues, but I fear adding complexity risks making it even worse.

-Kevin

@KevinOConnor KevinOConnor added the not mainline Wont merge into master branch label Mar 4, 2022
@github-actions
Copy link

This PR is being closed because it is currently not considered a good match for the master Klipper repository.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
not mainline Wont merge into master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants