davisp commented on a change in pull request #470: Scheduling Replicator URL: https://github.com/apache/couchdb/pull/470#discussion_r110432266
########## File path: src/couch_replicator/src/couch_multidb_changes.erl ########## @@ -0,0 +1,819 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_multidb_changes). +-behaviour(gen_server). + +-export([start_link/4]). + +-export([init/1, handle_call/3, handle_info/2, handle_cast/2]). +-export([code_change/3, terminate/2]). Review comment: Let the style review commence! I'm only picking on this module because its the first new module in the PR's diff view though this applies to all new modules and even old ones where we're changing things in the front matter and moving functions around to some degree. A module should have the following basic outline: We should work on organizing our export lists so that diffs are easier to read in the future. In general, we should have three export attributes for: public api, behavior callbacks, private module callbacks that need to be exported. Where behavior exports will have one attribute per behavior. Within an export we should have a single function per line. This way when these change the diff will be easier to read and it will be apparent on what's changing rather than our current status quo of sticking random functions into a module with randomly placed additions to the export attributes. For this module these should look something like: ```erlang -export([ start_link/4 ]). -export([ init/1, terminate/2, handle_call/3, handle_cast/2, handle_info/2, code_change/3 ]). -export([ changes_reader/3, changes_reader_cb/3 ]). ``` Also, the functions in the module should follow that order and any module private functions should follow after those in some logical order (I tend to prefer the order they were referenced by the exported functions so that its easy to know about where they are when searching for their definition, but that can be trumped if some other logical ordering makes sense, i.e., for trivial functions that aren't referenced often because their intent/implementation is obvious can usually just go as the very last functions in the module before any tests). Also a note on whitespace as there's quite a bit of variation throughout this PR. In general, there's a preference for two empty lines between sections and functions and a single empty line in places where it helps with readability. So, for instance, we're missing empty lines between the behavior attribute and the exports, and then between exports and includes, etc since these are different "sections". For the most part this PR uses two empty lines between functions but in some cases it only uses one or will even use three or four in some places. We'll want to normalize all of those. Places where single empty lines are helpful are when a handle_call/handle_cast function for a gen_server has a number of large clauses. A single empty line between clauses can be helpful to read each individually. Also in export lists it can be useful to group different related functions into sections delimited by empty lines so that related functions are obvious in their relation to each other. Above all though, lets try and be consistent. Also considering that a lot of these modules are new I'm being extra persnickety since these are greenfield and we're not doing things like requiring a style cleanup and then a change. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
