[GitHub] eiri commented on a change in pull request #1744: Fix total_rows value for queries on `_design_docs` handler

[GitBox <gi...@apache.org>]

eiri commented on a change in pull request #1744: Fix total_rows value for queries on `_design_docs` handler
URL: https://github.com/apache/couchdb/pull/1744#discussion_r234855111
 
 

 ##########
 File path: src/fabric/src/fabric_view_all_docs.erl
 ##########
 @@ -85,10 +85,13 @@ go(DbName, Options, QueryArgs, Callback, Acc0) ->
     Resp = case couch_util:get_value(namespace, Extra, <<"_all_docs">>) of
         <<"_local">> ->
             {ok, null};
-        _ ->
+        NS ->
 
 Review comment:
   Well, this is essentially just changing two-level `case` into one-level `case` and I agree this is indeed adds clarity, but as a trade off we are adding another level of abstraction with sending function as an argument (and those are usually annoying to debug) plus that awkward moment where we are sending `get_doc_count` function into `get_doc_counts` function, which I imagine would require some untangling on a casual code reading.
   
   How about abstracting things on `fabric:get_doc_count` level with something like this?
   
   ```diff
   diff --git a/src/fabric/src/fabric.erl b/src/fabric/src/fabric.erl
   index 36ed67154..868a72c46 100644
   --- a/src/fabric/src/fabric.erl
   +++ b/src/fabric/src/fabric.erl
   @@ -91,7 +91,13 @@ get_db_info(DbName) ->
        {error, atom()} |
        {error, atom(), any()}.
    get_doc_count(DbName) ->
   -    fabric_db_doc_count:go(dbname(DbName)).
   +    get_doc_count(DbName, <<"_all_docs">>).
   +
   +get_doc_count(DbName, <<"_all_docs">>) ->
   +    fabric_db_doc_count:go(dbname(DbName));
   +get_doc_count(DbName, <<"_design">>) ->
   +    fabric_design_doc_count:go(dbname(DbName)).
   +
    
    %% @equiv create_db(DbName, [])
    create_db(DbName) ->
   diff --git a/src/fabric/src/fabric_view_all_docs.erl b/src/fabric/src/fabric_view_all_docs.erl
   index ac16dac52..6a8b1546b 100644
   --- a/src/fabric/src/fabric_view_all_docs.erl
   +++ b/src/fabric/src/fabric_view_all_docs.erl
   @@ -85,10 +85,10 @@ go(DbName, Options, QueryArgs, Callback, Acc0) ->
        Resp = case couch_util:get_value(namespace, Extra, <<"_all_docs">>) of
            <<"_local">> ->
                {ok, null};
   -        _ ->
   +        Namespace ->
                Timeout = fabric_util:all_docs_timeout(),
                {_, Ref0} = spawn_monitor(fun() ->
   -                exit(fabric:get_doc_count(DbName))
   +                exit(fabric:get_doc_count(DbName, Namespace))
                end),
                receive {'DOWN', Ref0, _, _, Result} ->
                    Result
   ```

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services