-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add API method for getting all build_types regardless of conditions. #337
Conversation
This is more of a spike than anything. I'm not convinced this is the best way to go for this particular issue but it was the first thought that came to my head. Of possibly more concern is that this gets us past the "first hurdle" but I'm unclear why the simple version bump doesn't crash also.
0b54819
to
8871546
Compare
src/catkin_pkg/package.py
Outdated
@@ -166,6 +166,18 @@ def get_build_type(self): | |||
return build_type_exports[0] | |||
raise InvalidPackage('Only one <build_type> element is permitted.', self.filename) | |||
|
|||
def get_build_types(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cottsay should we rename this to something like get_unconditional_build_types()
or somesuch to make it more obvious that this is an "advanced" method?
Should we also or alternatively prefix it with _
?
def get_build_types(self): | |
def _get_unconditional_build_types(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would hurt, but I also think that adding docs to the function would be is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did decide to rename it without the _
prefix. Documentation for the method also indicates that conditions are not considered.
These patches were interleaved and #340 was not updated before being merged.
This is more of a spike than anything. I'm not convinced this is the
best way to go for this particular issue but it was the first thought
that came to my head.
Of possibly more concern is that this gets us past the "first hurdle"
but I'm unclear why the simple version bump doesn't crash also.