Draft: accept empty yaml_filename#2929
Conversation
|
@NikolasE, please properly fill in PR template in the future. @SteveMacenski, use this instead.
|
NikolasE
changed the title
SteveMacenski
left a comment
SteveMacenski
left a comment
There was a problem hiding this comment.
Mostly LGTM, 2-3 small updates.
There are some linting errors that CI brings up. Nothing crazy, just needs to be handled before merging
SteveMacenski
left a comment
SteveMacenski
left a comment
There was a problem hiding this comment.
@AlexeyMerzlyakov take a look and merge if you're happy
|
@NikolasE please un-draft this PR so it can be merged |
|
Actually @NikolasE you still need to fix linting issues, please look at CI's results |
|
@NikolasE any update? Just some really small changes / undrafting the PR to get this merged! |
|
Still on it, I just hadn't time to reproduce and fix the linting errors locally. |
AlexeyMerzlyakov
left a comment
AlexeyMerzlyakov
left a comment
There was a problem hiding this comment.
Overall, I am good with this change. There are small comments/nitpicks requested and we could go.
| on_configure or published during on_activate. The _load_map_-service should the be used to load and publish a map. | ||
|
|
||
|
|
||
| ## Map Saver |
There was a problem hiding this comment.
This name is intended to have 4 nesting level: here "Map Server" is a sub-chapter of "###CLI-usage" having nesting level equal to 3.
There was a problem hiding this comment.
Level 4 looks weird, 'Map Saver' is also on level two, but it's now on 4
| #include "nav2_util/lifecycle_service_client.hpp" | ||
| #include "nav2_msgs/srv/load_map.hpp" | ||
| using namespace std::chrono_literals; | ||
| using namespace rclcpp; |
There was a problem hiding this comment.
Do we really need rclcpp namespace?
There was a problem hiding this comment.
yes, mostly for Parameter and ParameterValue
|
@NikolasE any update? This is pretty close to getting in |
|
This pull request is in conflict. Could you fix it @NikolasE? |
|
I think everything remaining here are trivial linting fixes and then its good to go. If @NikolasE you don't have time for this, let us know and we can close this PR until a time when it can be completed to pass CI. CI picks up on half a dozen formatting issues, some of which @AlexeyMerzlyakov brought up in his PR review. See the CircleCI job for all the linting failures. |
|
Closing due to non-response. Happy to consider this / reopen it if you can make the minor changes @AlexeyMerzlyakov requested and are inline with our coding standards. |
|
#3038 completes |
3 participants
#2925
Empty yaml_filename is now default and node can be configured/activated without a valid map. loadmap can then be used to select a map.