Skip to content

Conversation

@JafarAbdi
Copy link
Contributor

  • Add jinja support for loading template yaml files
  • Add pre-commit to the readme

Copy link

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

First off, yay precommit!

I skimmed the code but it's too complex/lacks documentation/example/test for me to understand how it also solves the original problem of parsing !radians and !degrees.

Anything to help review?

def render_template(template: Path, mappings: dict):
with template.open("r") as file:
jinja2_template = jinja2.Template(file.read())
jinja2_template.globals["radians"] = math.radians

Choose a reason for hiding this comment

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

Is this what parses the !degrees and !radians in YAML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it adds a function to the jinja template

{% for robot in robots %}
{{ robot.name }}: "{{ robot.ip }}"
{% endfor %}
radians: {{ radians(120) }}

Choose a reason for hiding this comment

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

And are these parsed equivalently to !degrees/!radians?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, plus with jinja we could do more mathematical operations

@JafarAbdi
Copy link
Contributor Author

First off, yay precommit!

I skimmed the code but it's too complex/lacks documentation/example/test for me to understand how it also solves the original problem of parsing !radians and !degrees.

Anything to help review?

This PR adds a preprocessing step before loading files & yaml files by parsing a jinja template, this makes it possible to parameterize the config files (which is useful for moveit configs)

Thanks for taking a look, I have extended the unit tests & the example with two examples, do you think that I should add even more tests?

I agree that the package lacks some documentation. Maybe I'll spend sometime during WMD to improve it

@sea-bass
Copy link

Thanks for taking a look, I have extended the unit tests & the example with two examples, do you think that I should add even more tests?

I think if this PR can process yaml files with !degrees/!radians, like the jinja template parsing will pick that up and your test case is equivalent, no need for more tests. Just confirm this is the case and maybe leave a few comments.

I agree that the package lacks some documentation. Maybe I'll spend sometime during WMD to improve it

Nothing major needed! Just a few explanations for people like me :)

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