You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "iilyak (via GitHub)" <gi...@apache.org> on 2024/10/16 21:23:10 UTC
[PR] Add type specs to clouseau_rpc [couchdb]
iilyak opened a new pull request, #5309:
URL: https://github.com/apache/couchdb/pull/5309
## Overview
This PR serves as a code documentation since we don't run type checks during CI. It doesn't modify existing code just adding type specs.
## Testing recommendations
Since we don't run dyalizer during CI and given the fact that it fails on main. There is no good way to test correctness of the spec other than comparing the output of dilayzer on main and on the current branch.
on *main* branch
```
couchdb on *main*
❯ make
...
couchdb on *main*
❯ make dialyze apps=dreyfus
==> dreyfus (build-plt)
WARN: ''build-plt'' command does not apply to directory /Users/iilyak/Code/couchdb/rel
WARN: ''build-plt'' command does not apply to directory /Users/iilyak/Code/couchdb
==> dreyfus (dialyze)
src/dreyfus_bookmark.erl:61:29: Record construction
#shard{name :: '_', ref :: '_', opts :: '_'} violates the declared type of field opts ::
'undefined' | [any()]
src/dreyfus_httpd.erl:59:21: The pattern
{'ok', Bookmark0, TotalHits, Hits0} can never match the type
{'error', _} | {'ok', _, _, _, _, _}
src/dreyfus_index.erl:110:1: The inferred return type of init/1
('ok') has nothing in common with
'ignore' |
{'ok', _} |
{'stop', _} |
{'ok', _,
'hibernate' | 'infinity' |
non_neg_integer() |
{'continue', _}}, which is the expected return type for the callback of the gen_server behaviour
src/dreyfus_index.erl:152:36: The created fun has no local return
src/dreyfus_index.erl:231:36: The created fun has no local return
src/dreyfus_index_updater.erl:23:1: Function update/2 has no local return
ERROR: dialyze failed while processing /Users/iilyak/Code/couchdb/src/dreyfus: rebar_abort
make: *** [dialyze] Error 1
```
on *add-specs-to-clouseau_rpc* branch
```
couchdb on *add-specs-to-clouseau_rpc*
❯ make
...
couchdb on *add-specs-to-clouseau_rpc*
❯ make dialyze apps=dreyfus
==> dreyfus (build-plt)
WARN: ''build-plt'' command does not apply to directory /Users/iilyak/Code/couchdb/rel
WARN: ''build-plt'' command does not apply to directory /Users/iilyak/Code/couchdb
==> dreyfus (dialyze)
src/dreyfus_bookmark.erl:61:29: Record construction
#shard{name :: '_', ref :: '_', opts :: '_'} violates the declared type of field opts ::
'undefined' | [any()]
src/dreyfus_httpd.erl:59:21: The pattern
{'ok', Bookmark0, TotalHits, Hits0} can never match the type
{'error', _} | {'ok', _, _, _, _, _}
src/dreyfus_index.erl:110:1: The inferred return type of init/1
('ok') has nothing in common with
'ignore' |
{'ok', _} |
{'stop', _} |
{'ok', _,
'hibernate' | 'infinity' |
non_neg_integer() |
{'continue', _}}, which is the expected return type for the callback of the gen_server behaviour
src/dreyfus_index.erl:152:36: The created fun has no local return
src/dreyfus_index.erl:231:36: The created fun has no local return
src/dreyfus_index.erl:297:17: The variable Error can never match since previous clauses completely covered the type
{'ok', non_neg_integer()}
src/dreyfus_index_updater.erl:23:1: Function update/2 has no local return
ERROR: dialyze failed while processing /Users/iilyak/Code/couchdb/src/dreyfus: rebar_abort
make: *** [dialyze] Error 1
```
## Related Issues or Pull Requests
<!-- If your changes affect multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] Documentation changes were made in the `src/docs` folder
- [ ] Documentation changes were backported (separated PR) to affected branches
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type specs to clouseau_rpc [couchdb]
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak commented on PR #5309:
URL: https://github.com/apache/couchdb/pull/5309#issuecomment-2422731682
> Very nice, @iilyak !
>
> I noticed it has one extra error now after the types were added.
>
> ```
> src/dreyfus_index.erl:297:17: The variable Error can never match since previous clauses completely covered the type
> {'ok', non_neg_integer()}
> ```
>
> I wonder why that is? It would be nice if adding more types would reduce the discrepancies and show less errors not more.
This has been fixed by changing return type.
```diff
- {'ok', non_neg_integer()}
+ {'ok', non_neg_integer()} | error()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type specs to clouseau_rpc [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5309:
URL: https://github.com/apache/couchdb/pull/5309#discussion_r1806731912
##########
src/dreyfus/src/clouseau_rpc.erl:
##########
@@ -24,32 +24,138 @@
-export([set_purge_seq/2, get_purge_seq/1, get_root_dir/0]).
-export([connected/0]).
+%% string represented as binary
+-type string_as_binary(_Value) :: binary().
Review Comment:
There is [nonempty_binary/0](https://www.erlang.org/doc/apps/erts/erlang#t:nonempty_binary/0) as well now if that constrains the type better.
Tiny nit: wonder if `binary_string` is a bit shorter without the `as` in the middle?
As for "string" also, wonder they are plain 7-bit "ascii", utf8 encoded binaries, or a mix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type specs to clouseau_rpc [couchdb]
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak merged PR #5309:
URL: https://github.com/apache/couchdb/pull/5309
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type specs to clouseau_rpc [couchdb]
Posted by "iilyak (via GitHub)" <gi...@apache.org>.
iilyak commented on code in PR #5309:
URL: https://github.com/apache/couchdb/pull/5309#discussion_r1806869169
##########
src/dreyfus/src/clouseau_rpc.erl:
##########
@@ -24,32 +24,138 @@
-export([set_purge_seq/2, get_purge_seq/1, get_root_dir/0]).
-export([connected/0]).
+%% string represented as binary
+-type string_as_binary(_Value) :: binary().
Review Comment:
> As for "string" also, wonder they are plain 7-bit "ascii", utf8 encoded binaries, or a mix?
Good question.
We don't do anything with them. We just converting `list_to_binary` [here](https://github.com/apache/couchdb/blob/main/src/dreyfus/src/dreyfus_httpd.erl#L334). So we would get whatever the jiffy [would return](https://github.com/apache/couchdb-jiffy/blob/50daa80a62a97ffb6dd46ea9cb8ccb4930cbf1ae/c_src/decoder.c#L191).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type specs to clouseau_rpc [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5309:
URL: https://github.com/apache/couchdb/pull/5309#discussion_r1806693780
##########
src/dreyfus/src/clouseau_rpc.erl:
##########
@@ -24,32 +24,138 @@
-export([set_purge_seq/2, get_purge_seq/1, get_root_dir/0]).
-export([connected/0]).
+%% string represented as binary
+-type string_as_binary(_Value) :: binary().
+
+-type shard() :: string_as_binary(_).
+-type path() :: string_as_binary(_).
+
+-type error() :: any().
+
+-type analyzer_name() :: string_as_binary(_).
+
+-type field_name() :: string_as_binary(_).
+
+-type seq() :: non_neg_integer().
+-type commit_seq() :: seq().
+-type purge_seq() :: seq().
+-type committed_seq() :: seq().
+-type pending_seq() :: seq().
+-type update_seq() :: seq().
+
+-type analyzer_fields() :: [{field_name(), analyzer_name() | [analyzer_name()]}].
+
+-type indexer_pid() :: pid().
+
+%% Example of the message
+%% {[
+%% {<<"name">>,<<"perfield">>},
+%% {<<"default">>,<<"keyword">>},
+%% {<<"fields">>,{[
+%% {<<"$default">>,<<"standard">>}
+%% ]}}
+%% ]}
+-type analyzer() ::
+ analyzer_name()
+ | [
+ {string_as_binary(name), analyzer_name()}
+ | {string_as_binary(default), analyzer()}
+ | {string_as_binary(fields), {analyzer_fields()}}
+ | {string_as_binary(stopwords), [field_name()]}
+ ].
+
+-spec open_index(Peer :: pid(), Path :: shard(), Analyzer :: analyzer()) ->
+ {ok, indexer_pid()} | error().
open_index(Peer, Path, Analyzer) ->
rpc({main, clouseau()}, {open, Peer, Path, Analyzer}).
+-spec disk_size(Path :: shard()) ->
+ {ok, {disk_size, non_neg_integer()}} | error().
+
disk_size(Path) ->
rpc({main, clouseau()}, {disk_size, Path}).
+
+-spec get_root_dir() ->
+ {ok, path()} | error().
+
get_root_dir() ->
rpc({main, clouseau()}, {get_root_dir}).
+%% not used ???
+-spec await(Ref :: indexer_pid(), MinSeq :: commit_seq()) ->
+ ok | error().
+
await(Ref, MinSeq) ->
rpc(Ref, {await, MinSeq}).
+%% deprecated
+-spec commit(Ref :: indexer_pid(), NewCommitSeq :: commit_seq()) ->
+ ok | error().
+
commit(Ref, NewCommitSeq) ->
rpc(Ref, {commit, NewCommitSeq}).
+-type info_result_item() ::
+ {'disk_size', non_neg_integer()}
+ | {'doc_count', non_neg_integer()}
+ | {'doc_del_count', non_neg_integer()}
+ | {'pending_seq', pending_seq()}
+ | {'committed_seq', committed_seq()}
+ | {'purge_seq', purge_seq()}.
+
+-spec info(Ref :: indexer_pid()) ->
+ {ok, [info_result_item()]} | error().
+
info(Ref) ->
rpc(Ref, info).
+-spec get_update_seq(Ref :: indexer_pid()) ->
+ {ok, update_seq()} | error().
+
get_update_seq(Ref) ->
rpc(Ref, get_update_seq).
+-spec set_purge_seq(Ref :: indexer_pid(), Seq :: purge_seq()) ->
+ ok | error().
+
set_purge_seq(Ref, Seq) ->
rpc(Ref, {set_purge_seq, Seq}).
+-spec get_purge_seq(Ref :: indexer_pid()) ->
+ {ok, purge_seq()} | error().
+
get_purge_seq(Ref) ->
rpc(Ref, get_purge_seq).
+-type query() :: string_as_binary(_).
+-type range_name() :: string_as_binary(_).
+-type range_label() :: string_as_binary(_).
+-type range_query() :: string_as_binary(_).
+-type partition() :: string_as_binary(_).
+
+-type limit() :: non_neg_integer().
+-type bookmark() :: string_as_binary(_).
+
+-type search_arg() ::
+ {'query', query()}
+ | {'partition', partition()}
+ | {'limit', limit()}
+ | {'refresh', boolean()}
+ | {'after', bookmark()}
+ | {'sort', group_sort()}
+ | {'include_fields', [field_name()]}
+ | {'counts', [field_name()]}
+ | {'ranges', [{range_name(), [{range_label(), range_query()}]}]}
+ | {'highlight_fields', [field_name()]}
+ | {'highlight_pre_tag', string_as_binary(_)}
+ | {'highlight_post_tag', string_as_binary(_)}
+ | {'highlight_number', pos_integer()}
+ | {'highlight_size', pos_integer()}
+ | {'legacy', boolean()}.
Review Comment:
Let's not use quotes unless we have to. I don't think we quote atoms in any other types in the code-base so having one place stand-out looks odd.
For instance:
https://github.com/apache/couchdb/blob/main/src/mem3/src/mem3_reshard.hrl#L22-L44
https://github.com/apache/couchdb/blob/main/src/fabric/include/fabric.hrl#L40-L47
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type specs to clouseau_rpc [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #5309:
URL: https://github.com/apache/couchdb/pull/5309#issuecomment-2422745292
> This has been fixed by changing return type.
>
> ```diff
> - {'ok', non_neg_integer()}
> + {'ok', non_neg_integer()} | error()
> ```
Ah, very nice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type specs to clouseau_rpc [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5309:
URL: https://github.com/apache/couchdb/pull/5309#discussion_r1806731912
##########
src/dreyfus/src/clouseau_rpc.erl:
##########
@@ -24,32 +24,138 @@
-export([set_purge_seq/2, get_purge_seq/1, get_root_dir/0]).
-export([connected/0]).
+%% string represented as binary
+-type string_as_binary(_Value) :: binary().
Review Comment:
There is [nonempty_binary/0](https://www.erlang.org/doc/apps/erts/erlang#t:nonempty_binary/0) as well now if that constrains the type better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org