On 3/9/2016 6:22 AM, Radomir Dopieralski wrote:
Some of the code we have in Nova is admittedly hard to read and brittle. This is true even though we cave excellent code review, and the reviewers that deeply care for the quality of code that is being merged. Some of this code has been there always, some got that way as a result of bug fixes and patches that were intended to touch as little code as possible, and some is just to bad judgment or mistakes. Whatever the reason, such code accumulates and makes the program harder to understand, to debug and to modify. The maintenance and development costs rise. It's also less pleasant to work with such code base. Cleaning up such code and rewriting it to be easier to understand and more robust is a good practice that every programmer should be familiar with. However, it's not always easy or even possible to do that while working on a feature or a bugfix. If our patch includes a lot of changes that are unrelated to the feature or bug at hand, it becomes harder to understand and review, and even runs a risk of being rejected. It's also easier to introduce bugs that way. A good practice would be to make such changes in separate patches, possibly also adding tests (where they are missing) showing that there is no change in behavior. Such patches can be reviewed with special attention to readability and robustness, and in the review there will be often even further improvements. We end up with a better program and easier (and more pleasant) work. And now, finally, I can get to the point of this e-mail. I'm relatively new to this project, but I found no way to direct the (precious) attention of core reviewers to such patches. They are not bugs, neither they are parts of any new feature, and so there is no list in Nova that core reviewers look at where we could add such a patch. Admittedly, such patches are not urgent -- the code works the same (or almost the same) way without them -- but it would be nice to have some way of merging them eventually, because they do make our lives easier in the long run. So here's my question: what is the correct way to have such a patch merged in Nova, and -- if there isn't one -- should we think about making it easier?
I don't think we give any special priority to technical debt cleanup unless it's part of a larger series, like cleaning up code before fixing a bug or landing a feature.
So you can and should push patches to refactor code to make it more maintainable/readable/etc, but there isn't a guarantee that it's going to get attention any sooner than every other patch out in the review queue (according to stackalytics there are currently 842 open reviews in nova [1]).
[1] http://stackalytics.com/report/reviews/nova/open -- Thanks, Matt Riedemann __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
