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 lunar to version macro, possible fix for eol distros in status bar #169
Conversation
Some of the changes in this PR should be deployed now (e.g. updating hidden distros), some other only after Lunar has been released (e.g. active distro). Therefore those should be separated into two PRs. |
10eb7cd
to
252ef13
Compare
The lunar specific changes have been removed to be added in a follow-up. |
hidden_distros = ['boxturtle', 'cturtle', 'diamondback', 'electric', 'fuerte', 'groovy', 'hydro', 'unstable'] | ||
|
||
for hidden_distro in hidden_distros if hidden_distro in distro_names: | ||
distro_names.remove(hidden_distro) |
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 would suggest using list comprehension for this:
distro_names = [d for d in distro_names if d not in hidden_distros]
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.
done thanks
9d44307
to
1a62c4c
Compare
It doesn't have to be address in this PR but if you search for e.g. Lines 14 to 16 in b6e2236
|
distros.remove('boxturtle') | ||
if 'unstable' in distros: | ||
distros.remove('unstable') | ||
distros = [d for d in distro_names if d not in distro_names_hidden] |
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 would expect this breaks the use of the Version
macro for "hidden" distros.
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.
Ok, I think I misunterpreted what you meant when you said
but if you search for e.g. boxturtle in this repo there are numerous locations where specific distro names are being ignored
could you clarify how this ties to declaring the list of hidden distros in macroutils ? do you mean creating another list for the subset of distros ignored in version?
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.
The commit where you moved the hidden distros in a single location is good as is. The package and stack headers now use the same list of non-hidden distros.
This case is different for the version macro. It is being used within the content of wiki pages and marks e.g. a paragraph describing a certain feature with the ROS distro when it was added. So it needs to continue to work for all distros. I don't know why this should exclude "boxturtle" and "unstable" (but I wouldn't touch this).
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.
got it, thanks
#169 (comment) has been addressed |
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.
lgtm
The change needs to be deployed to the server after being merged. |
had to keep the alteration of the |
I believe this will hide EOL'd distros from the package pages, but I'm not sure if this is the solution I'd like to settle on.