Opinionated code review

[tkruse]

Hi William, so I took some time today to look over catkin_tools to see what good use of a marker file could be integrated. I took down notes of things i noticed that I would have done differently. Those are not not defects, just opinions, so feel free to just disagree, I will be happy to agree to disagree.

• [ ] missing contributor docs (how to set up local environment to run catkin_tools main without having to install after each change. I don't know how you do it, I put "if name=='main'... in catkin.py)
• [ ] Define custom Exception that is printed without StackTrace for normal failure cases(anything that does not look like a bug in the code), ValueError is for lazy-bodies in such cases. E.g. Could not find source space: ... StackTraces are scary, they scare users, they should only be printed where a bug is likely, e.g. the user has to contact the issue tracker or answers.ros.org to find help.
• [ ] unit test coverage... nuf said
• [ ] I like colors as much as the next guy, but seriously: Don't use colors. Unless for exceptional cases (failure = red). catkin_tools should later be maintainable by anyone who has no interest whatsoever in learning the syntax rules of ansi. Every minute spend on fixing some ansi related issue is a minute lost on serious stuff that advances mankind. On demo day, 3 minutes before the demo, you do not want a build to fail mysteriously due to a bug with the ansi formatting. It's just not worth it.
• [ ] color.py:_color_translation_map: No. If you write such code, that's when you know you are using colors too much. Since most of the items in the map seem to do modular replacements, e.g. ([{package}] -> [@{cf}{package}@|], [ {time} ] -> [ @{yf}{time}@| ]), maybe you could just write def format_packagename(), or have a DSL for formatting special blocks of text. Else just use plain black and white, really.
• [ ] def fmt(), def clr(): use meaningful names. Meaningful to the uninitiated reader, not you. I know, you use those often, typing causes finger strain, etc. Still, use meaningful names, no exception.
• [ ] build.py:build_isolated_workspace(): function too long. cut it down for unit testing. also see point "unit test coverage" above. unit testing long functions is much more of a pain than small functions.
• [ ] catkin.py:main(): function too long. Also do not both use argparse and manually check arguments in a for loop, or place a code comment explaining why you do it.
• [ ] context.py: if self.__locked:... : duplicate code, maybe use def check_lock()...?

Else do not consider this as criticism, I like the initiative behind catkin_tools, so keep up the good work.

[wjwwood]

missing contributor docs (how to set up local environment to run catkin_tools main without having to install after each change. I don't know how you do it, I put "if name=='main'... in catkin.py)

I think that this is documented here: http://catkin-tools.readthedocs.org/en/latest/#developing

The reason there is no catkin script file is because it uses the entry_points system provided by setuptools, which is described in part here: http://catkin-tools.readthedocs.org/en/latest/development/extending_the_catkin_command.html

Define custom Exception that is printed without StackTrace for normal failure cases(anything that does not look like a bug in the code), ValueError is for lazy-bodies in such cases. E.g. Could not find source space: ... StackTraces are scary, they scare users, they should only be printed where a bug is likely, e.g. the user has to contact the issue tracker or answers.ros.org to find help.

It's true the error handling behavior is unpolished, didn't have time to do that yet.

I do agree though that for any user error modes that it might be good to have custom exceptions to make catching particular scenarios easier.

unit test coverage... nuf said

I agree, but the sponsor which paid for the development of this tool originally had a tight timeline (I wrote the vast majority in four days to meet their needs) and so they are missing.

At this point I wanted to make sure the community benefitted from our work with Google, so for me "finished without tests" is better than "not finished with some tests".

Obviously, if I had more time I would cover it, as I have when working on projects with proper support:

https://coveralls.io/r/osrf/capabilities

I like colors as much as the next guy, but seriously: Don't use colors. Unless for exceptional cases (failure = red). catkin_tools should later be maintainable by anyone who has no interest whatsoever in learning the syntax rules of ansi. Every minute spend on fixing some ansi related issue is a minute lost on serious stuff that advances mankind. On demo day, 3 minutes before the demo, you do not want a build to fail mysteriously due to a bug with the ansi formatting. It's just not worth it.

Obviously I disagree, but I think that you miss the point that colorization is not just pretty or superficial. It allows users to match patterns more quickly and to extract pertinent information out of formatting or context more easily. I personally think that most tools which use colors in this way are better for it:

• git
• Homebrew
• Every modern text editor ever
• clang
• ls
• my shell prompt

I will concede that I use colors in places that they are not absolutely necessary, but once I have the infrastructure to do colorization, it is easy to add and adds some polished feel to the tool.

I will also point out that I always test without colors on to make sure the tool is as readable as possible using only whitespace and content separating characters.

color.py:_color_translation_map: No. If you write such code, that's when you know you are using colors too much. Since most of the items in the map seem to do modular replacements, e.g. ([{package}] -> [@{cf}{package}@|], [ {time} ] -> [ @{yf}{time}@| ]), maybe you could just write def format_packagename(), or have a DSL for formatting special blocks of text. Else just use plain black and white, really.

I disagree, this is exactly (mostly) the pattern used by tools like gettext for localization. I adopted the idea that colorization is just a form of translation. I did this in an effort to hide the colorization from the codebase to allow people to, in your words, "be maintainable by anyone who has no interest whatsoever in learning the syntax rules of ansi". With this system they can. If they change something it may cause colorization to stop working, but the same could be true for localization.

I think this would be a good strategy regardless of how much colorization is used, and therefore does not indicate the degree to which colors are used.

def fmt(), def clr(): use meaningful names. Meaningful to the uninitiated reader, not you. I know, you use those often, typing causes finger strain, etc. Still, use meaningful names, no exception.

I agree, these are held over from a go colorization library which I converted to Python and that's where I got the annotation scheme, e.g. @{redf}red@|. Also, I was trying to keep the line length down to comply with PEP8, but I think there are better ways to do that. I am in the process of refactoring this to be format_color and translate, where translate will be used for localization and colorization, i.e. choose from ('en-US', 'en-US+color', etc...).

build.py:build_isolated_workspace(): function too long. cut it down for unit testing. also see point "unit test coverage" above. unit testing long functions is much more of a pain than small functions. catkin.py:main(): function too long.

I agree to both.

Also do not both use argparse and manually check arguments in a for loop, or place a code comment explaining why you do it.

I wouldn't if it were not strictly required. The reasoning is in the documentation about extending the catkin command:

http://catkin-tools.readthedocs.org/en/latest/development/extending_the_catkin_command.html (see stuff about argument_preprocessor)

Because of the argument_preprocessor feature, which is needed for catkin build, I have to do verb processing outside of argparse. Maybe you could be more specific as to where the comments and code together are not revealing enough.

context.py: if self.__locked:... : duplicate code, maybe use def check_lock()...?

That should just be refactored, I was locking the context, but I do not believe it is necessary anymore.

[tkruse]

Thanks for answering in detail. I had no idea setup.py develop existed, and must have been blind for it. Note it could mention virtualenv, because without virtualenv sudo seems to be required on my system.

Regarding colors, I only commented on the negative effect it has on code maintainability. I can also comment on functional effects. There are real benefits to usability (which can objectively be measured by stopping the time a user can answer questions about the output), but there are also adverse effects. Concerns are about supporting both dark-on-light as well as light-on-dark themes, supporting non-standard terminals, confusing the user and considering the color-blinds. Also colors reduce contrast to the brackground, making things easier to detect, but harder to read. For catkin_tools, bright cyan is a real pain for me to read on my dark-on-light theme. I think the main use-cases for colors are:

• friendly banners
• highlighting error messages
• clustering sets of text item belonging together semantically
• provide visual patterns to identify re-occuring situations quickly

I am thinking mostly aloud here, not lecturing, I am curious as to what you think about the following set of guidelines for color usage in shell tools in general. I tried to visit colored shell tools I like and reverse-engineer what principles they could have used, and see whether I can find rules that seem reasonable that catkin_tools violates.

1. default style (non-color, non-bold) offer the best constrast and readability (that's why it is default in the first place), it should be used for longer stretches of text (paths, sentences), unless the text can mostly be ignored
2. styles for highlighting (attention-grabbing) only work when they are the exception. If 90% of elements on a page have individual styles, none of them grab attention, they all work together to divert attention.
3. colors should work well on light-and-dark and dark-and-light themes
4. Same elements should be styled the same wherever they appear (consistency)
5. Items with relatively little informational value should be default styled in order not to grab attention away from more important items
6. items that belong together should have the same style

If you can add more rules based on how you select colors, I would be interested. If you don't have time, that's fine with me.

Based on these rules I would change catkin_tools coloring as follows:

• Do not bolderize paths (1.)
• Use same style for packagename eveywhere (2., 4.)
• Do not color arrows ==> <== (2., 5.)
• If coloring arrows, color them consistently (2., 4., 5.)
• Do not bolderize "Finished" (2., 5.)
• Do not bolderize 'in' (2., 5.)
• Do not use cyan, nor light-green, probably no light color at all (3.)
• Do not use multiple styles within "failed with return code '2'" (2., 6.)
• Do not present package ames with different styles (4.)

Following the principles would then equally have a beneficial effect on the code.

Again, just offering my opinion, smart use of colors can give a shell tool a well-polished look ("Look at that, they even considered colors, and it helps"), but excess colors give a tool a gaudy immature look ("They are begging for my attention, clearly they used colors more for their benefit than mine. The tool makes itself appear too important. They had the infastructure for colors in place, so they just painted away because they could.").

Given that you always test readability is good even without colors, many colors will necessarily look like fooling around.

Again, answer in the briefest way if you are not interested, I wrote this mostly to find my own stance on the functional benefits and risks of coloring.