Added support for _and_newer and _and_older suffixes in Version macro #127
Conversation
|
@dirk-thomas @trainman419 @tfoote @esteve for review. |
|
If merged, I will update this page: http://wiki.ros.org/WikiMacros#Version |
|
+1 |
|
+1 . The API and the documentation looks great. I don't really understand the moinmoin macro system, so I don't feel qualified to review the implementation. |
|
One note, I couldn't use my first choice of |
| @@ -36,16 +55,40 @@ def execute(macro, args): | |||
| 'New in %s</span>' % version) | |||
|
|
|||
| def distro_html(distro, distros): | |||
| active = [distro.encode("iso-8859-1")] | |||
| inactive = [x.encode("iso-8859-1") for x in distros if not x == distro] | |||
| html = "" | |||
There was a problem hiding this comment.
May be remove this line and assign it in line 80? Makes the variable have much smaller scope.
There was a problem hiding this comment.
I don't think it changes the scope; I had that there for debugging by adding more stuff to the html in the loop. I'll move it, but I don't see that it matters.
|
Comments addressed, @tfoote can you spare a second to review this. I'd like to start using it on the wiki asap. |
| assert(distro in distros) | ||
| distro_index = sorted_distros.index(distro) | ||
| preceding_distros.extend(sorted_distros[:distro_index]) | ||
| proceeding_distros.extend(sorted_distros[distro_index:][1:]) |
There was a problem hiding this comment.
Why not remove line 58 and 59 and replace 64 and 65 with the following?
preceding_distros = sorted_distros[:distro_index + 1]
proceeding_distros = sorted_distros[distro_index:]
|
lgtm |
|
Thanks, @esteve can you merge and deploy? |
Added support for _and_newer and _and_older suffixes in Version macro
fixes the $ROS_DISTRO js based "macro" after #127
Previously you had to explicitly state for each block which ROS distro's were applicable, but now you can do something like this:
Fixes: #110