Catkin plugin: Refactor build. #175
Conversation
kyrofa
force-pushed
the
catkin_refactor_build
branch
from
b04905a
to
ba0eb40
Compare
| @@ -25,6 +25,9 @@ | |||
| - catkin-packages: | |||
| (list of strings) | |||
| List of catkin packages to build. | |||
| - source-space: | |||
There was a problem hiding this comment.
Is source-space the word used upstream, if not, it feels weird. Maybe just workspace?
There was a problem hiding this comment.
Yeah, line 262. Workspace would be confusing since that's what source/source-subdir should be pointing to (the catkin workspace). Catkin has a "build space," a "devel space," an "install space," and a "source space." And a workspace 😛 .
|
💯 on this, looks really good in the readability domain! Just had a couple nitpicks here and there. |
kyrofa
force-pushed
the
catkin_refactor_build
branch
from
ba0eb40
to
a3a7420
Compare
|
Thanks for the good review @sergiusens! I've pushed a new version that should address your concerns. |
|
Looks good, just adding one more comment about you have my 👍 regardless |
kyrofa
force-pushed
the
catkin_refactor_build
branch
from
a3a7420
to
4ef6aa7
Compare
|
@sergiusens Fixed both. You're right, the |
This commit contains a number of small refactors: - Change env() to only contain variables for run-time, and document each of them. - Only obtain dependencies upon pull()-- for build() they should just be ready to go. - Extract dependency resolution into a standalone function that can be tested in isolation. - Change build() to be less clever and more simple/verbose/robust. - Add `source-space` keyword to get rid of custom build(). - Get testing coverage to 100%. Signed-off-by: Kyle Fazzari <kyle@canonical.com>
kyrofa
force-pushed
the
catkin_refactor_build
branch
from
4ef6aa7
to
9045874
Compare
Catkin plugin: Refactor build.
Fixed typo
Catkin plugin: Refactor build.
This PR contains a number of small refactors:
env()to only contain variables for run-time, and document each of them.pull()-- forbuild()they should just be ready to go.build()to be less clever and more simple/verbose/robust.source-spacekeyword to get rid of custombuild().