Erb template - #40
Conversation
| module_function | ||
|
|
There was a problem hiding this comment.
Avoid this. It's very easy to miss it when the module gets bigger.
There are two better patterns:
- define each method with
self., e.g.def self.parse_erb_as_str - define the module normally and do an
extend selfat the very end
The latter is also hard to miss, but the module in itself behaves normally (one can include it elsewhere and it works). I prefer (1) when the purpose of the module is really to be a namespace.
| @@ -0,0 +1,32 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Please split code in modules only when you see that things get too big, or that you need the functionality in multiple places. Not when you think it/you will.
The functionality of erb.rb should really be straight into ERBLoader#parse_sdf_document
There was a problem hiding this comment.
Ok, I will include it in the ERBLoader class.
| @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
|
|
||
| module SDF | ||
| # class to load SDF and ERB templated SDF files | ||
| class ERBLoader < Loader |
There was a problem hiding this comment.
I'm missing the purpose or advantage of having separate loader and ERBLoader classes. This stuff is so simple, why not a single class ?
There was a problem hiding this comment.
If I remember correctly, @jhonasiv requested me to do it so we could enforce that when the SDF::Loader is configured only .sdf files are loaded
| raise Errno::ENOENT, | ||
| "Cannot find '#{file_name}' in '#{dir_path}'." \ | ||
| "You probably want to update the GAZEBO_MODEL_PATH " \ | ||
| "environment variable, or set SDF.model_path explicitly." |
There was a problem hiding this comment.
This class knows nothing about GAZEBO_MODEL_PATH or SDF.model_path. It is given an already resolved path as argument.
You should assume that the argument exists and let ENOENT propagate. The levels that do resolution based on GAZEBO_MODEL_PATH should error out if a model does not exist.
| "#{sdf_file} can be parsed as an XML file, but it " \ | ||
| "does not have a root" | ||
| end | ||
| return if %w[sdf gazebo].include?(sdf.root.name) |
There was a problem hiding this comment.
gazebo as a root ? Is that valid SDF ?
There was a problem hiding this comment.
I don't think so: https://sdformat.org/spec/1.12/sdf/
I included it because it was in the original code and I didn't know whether there was a reason for it.
I will remove it then
| "Cannot load #{sdf_file}: #{e.message}" | ||
| end | ||
|
|
||
| REXML::Document.new |
There was a problem hiding this comment.
I know that it was in the original code, but ...
I think the value of having an error message that says "it loads as XML but it has no root" rather than "this file has no root" does not warrant the complexity of having a codepath dedicated for it. Please simplify it.
| require_relative "exceptions" | ||
| require_relative "sdf_loader" | ||
| require_relative "erb_loader" |
There was a problem hiding this comment.
We do not use require_relative.
There was a problem hiding this comment.
Honest question, why is that? I thought require_relative was less error prune
| @@ -0,0 +1,64 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
SDF::Loader lives in sdf/loader.rb not sdf/sdf_loader.rb
3 participants
Add SDF::Loader and SDF::ERBLoader and use loader in
load