Erb template - #40
Conversation
| @erb_args = erb_args | ||
| end | ||
|
|
||
| def parse_sdf_document(sdf_file) |
There was a problem hiding this comment.
I think you should make a difference between erb and non-erb files, that is parse ERB only when the extension is .erb
There was a problem hiding this comment.
I told him to do it like this. If you mean for having an explicit error when a non-erb file is given to an ERBLoader, I feel this is overkill and it would painful in the Robot level to constantly juggle between loaders when the model file changes (I dont think you mean this, just getting it out there).
In the case you want to split the functionality between parse_sdf_document, or do a plain load directly as its done nowadays when the file does not have a .erb, my concern would be the flakiness of someone defining a model.sdf that IS an file with ERB variables on it without realizing, and then the syntax error when interpreting the SDF would be probably very noisy.
There was a problem hiding this comment.
I agree with @jhonasiv on this one, but in this case I would simply remove the base SDF::Loader class and keep only the ERBLoader.
In case we enforce the files to end with .erb then I suggest we keep both loaders and make them only handle their specific file extension
But anyway, I don't have a strong opinion on this, so I would happily go with any
There was a problem hiding this comment.
A part from this comment, the review was addressed. I didn't address this one as it seems there is no consensus yet, let me know what to do about this one
There was a problem hiding this comment.
I strongly want the ERB files to have .erb, and to allow us to have non-erb files as well as erb files in the same overall configuration.
This comes from experience (the orogen configuration files .yml for which I did not make it explicit, something I regret)
- editors have templating syntax highlighting. Adding
.erbmakes sure the editor syntax-highlights ruby code in the ERB markers and uses XML outside of it. - processing
.erbfiles is not cheap. - having the extension makes it clear that the file is actually a template. Having to figure it out by opening and checking whether there are ERB markers is error-prone (and annoying).
- changing to/from erb template to non-erb template is a local change (just change the .config and rename the file), so I don't see a strong argument regarding this either.
The whole "having different loaders" because of this makes no sense to me. The difference between ERB and non-ERB files is minimal (the ERB processing itself). Actually having a single loader makes sure we align both codepaths behaviours.
There was a problem hiding this comment.
I understand @jhonasiv's concern about the errors though.
But now, having the separation .sdf / .sdf.erb also makes sure that tooling that knows nothing about .sdf (a.k.a. "non-rock tooling") isn't accidentally given something they can't handle.
There was a problem hiding this comment.
Validated that ERB markers are invalid XML. We could handle invalid XML in the .sdf path and check for these markers to improve error reporting ("error blablabla, if it is a ERB template, add a .erb extension and update the .config file)
| @erb_args = erb_args | ||
| end | ||
|
|
||
| def parse_sdf_document(sdf_file) |
There was a problem hiding this comment.
I strongly want the ERB files to have .erb, and to allow us to have non-erb files as well as erb files in the same overall configuration.
This comes from experience (the orogen configuration files .yml for which I did not make it explicit, something I regret)
- editors have templating syntax highlighting. Adding
.erbmakes sure the editor syntax-highlights ruby code in the ERB markers and uses XML outside of it. - processing
.erbfiles is not cheap. - having the extension makes it clear that the file is actually a template. Having to figure it out by opening and checking whether there are ERB markers is error-prone (and annoying).
- changing to/from erb template to non-erb template is a local change (just change the .config and rename the file), so I don't see a strong argument regarding this either.
The whole "having different loaders" because of this makes no sense to me. The difference between ERB and non-ERB files is minimal (the ERB processing itself). Actually having a single loader makes sure we align both codepaths behaviours.
| @erb_args = erb_args | ||
| end | ||
|
|
||
| def parse_sdf_document(sdf_file) |
There was a problem hiding this comment.
I understand @jhonasiv's concern about the errors though.
But now, having the separation .sdf / .sdf.erb also makes sure that tooling that knows nothing about .sdf (a.k.a. "non-rock tooling") isn't accidentally given something they can't handle.
| @erb_args = erb_args | ||
| end | ||
|
|
||
| def parse_sdf_document(sdf_file) |
There was a problem hiding this comment.
Validated that ERB markers are invalid XML. We could handle invalid XML in the .sdf path and check for these markers to improve error reporting ("error blablabla, if it is a ERB template, add a .erb extension and update the .config file)
| # Parses an ERB string and returns the raw rendered string | ||
| # | ||
| # @param [String] erb_content ERB template file content as string | ||
| # @param [Hash] erb_args the configuration arguments to evaluate | ||
| # @return [String] the raw rendered XML string representing the model | ||
| def self.parse_erb_as_str(erb_content, **erb_args) | ||
| erb_engine = ::ERB.new(erb_content, trim_mode: "-") | ||
|
|
||
| # Render the ERB template with the passed hash arguments | ||
| erb_engine.result_with_hash(erb_args) | ||
| end | ||
|
|
||
| # Renders an ERB template and returns it as a REXML::Document | ||
| # | ||
| # @return [REXML::Document] the rendered sdf model | ||
| def self.render_erb_sdf_model(path, **erb_args) | ||
| erb_content = File.read(path) | ||
| solved_erb_as_sdf_str = parse_erb_as_str(erb_content, **erb_args) | ||
|
|
||
| REXML::Document.new(solved_erb_as_sdf_str) | ||
| end |
There was a problem hiding this comment.
This really is "over-splitting". Including parse_sdf_document, you are actually splitting 4 lines of code into 3 methods.
3 participants
Add SDF::Loader and SDF::ERBLoader and use loader in
load