You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2015/01/30 00:14:41 UTC
[GitHub] couchdb-config pull request: 2561 make config api consistent
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-config/pull/2
2561 make config api consistent
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/iilyak/couchdb-config 2561-make-config-API-consistent
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-config/pull/2.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2
----
commit e19b7d5c02b70e14138c9fe1e21f2650100d0a21
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-01-29T20:52:29Z
Remove tests for dropped register functionality
COUCHDB-2561
commit ee24f3ef1a40c1441852dea41061998ac30eb3de
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-01-29T20:53:11Z
Change setup/teardown logic and enable some tests
COUCHDB-2561
commit 82711bfbe91b1dde100d88c9469384ca1dcd564d
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-01-29T20:55:27Z
Callback driven API for config_listener behaviour
This implementaion replaces a mix of message driven and callback driven
API for config_listener. In particular it replaces gen_event_EXIT
message with a call to Module:handle_config_stop(Pid, Reason, State).
This fixes the problem of using config:listen_for_changes in supervisor
context where there is no way to handle arbitrary messages.
COUCHDB-2561
commit 29d9eaea7255f7f333ec0dec2b3d561236d3be70
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-01-29T21:01:21Z
Enforce type verification for config:set/get
Check the type of given default value to make sure it is supported.
Raise error(badarg) from set/get on type missmatch.
Add tests for the feature
COUCHDB-2561
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72159638
Awesome work, but I'm in doubt is that reasonable to disallow atoms as Default value for `config:get`. By looking on referenced PRs you'd change a lot of `config:get` routines, and...let's pick this one from couchdb-couch:
```diff
- case config:get(<<"httpd">>, <<"config_whitelist">>, null) of
- null ->
+ case config:get("httpd", "config_whitelist", "null") of
+ "null" ->
% No whitelist; allow all changes.
handle_approved_config_req(Req, Persist);
WhitelistValue ->
```
So, for old behaviour having `[httpd] config_whitelist = null` was not allowed. Now it's ok. So actually these changes introduced yet another state of deleted options in config INI file - when it matches the default in code (most defaults are nil and null). Atoms are good sentinel values for "no value" case and we should use them, not abuse.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72264482
I hope some day we can deprecate `config:get/2` so every caller would have to provide sensible default.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72218535
I'll push new commit using `undefined`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72216258
@iilyak hm...true. Need to update those code after your PRs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72672077
@iilyak all cool. one more thing before merge: could you please squash renaming and config default changes and misc fixing commits (indent in couchdb-couch)? We don't need to reflect changes in history that were instantly changed in the following commits (e.g. `nil -> "nil" -> undefined` when the only change that matters is `nil -> undefined` etc.).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72218702
@iilyak btw, it seems you'd missed ioq project. I made a change in my branch. Could you take a look on it? https://github.com/kxepal/couchdb-ioq/commit/b7b841294f7a37d9e9b59d2833e781e36bcad236
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72878701
All merged! Thank you! \o/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72215561
The problem with `[httpd] config_whitelist = null` is very easy to solve using either.
```
case config:get(<<"httpd">>, <<"config_whitelist">>, undefined) of
undefined ->
```
OR
```
case config:get(<<"httpd">>, <<"config_whitelist">>) of
undefined ->
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-config/pull/2
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72257827
All PRs are updated. Summary of updates:
1 rename `handle_config_stop` into `handle_config_terminate`
2. make use of `undefined` default value when calling `config:get/3`
3. Add `handle_config_terminate(_, stop, _) -> ok;` clause to prevent event handler restart on termination
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72863547
All dependent PRs are cleaned up and ready for merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-config/pull/2#discussion_r23826022
--- Diff: test/config_tests.erl ---
@@ -465,3 +358,92 @@ should_create_persistent_option() ->
ok = config:set("httpd", "bind_address", "127.0.0.1"),
config:get("httpd", "bind_address")
end).
+
+should_handle_value_change({Pid, _}) ->
+ ?_test(begin
+ ok = config:set("httpd", "port", "80", false),
+ ?assertMatch({{"httpd", "port", "80", false}, _}, wait_reply(Pid))
+ end).
+should_pass_correct_state_to_handle_config_change({Pid, _}) ->
+ ?_test(begin
+ ok = config:set("httpd", "port", "80", false),
+ ?assertMatch({_, {Pid, _, []}}, wait_reply(Pid)),
+ ok = config:set("update_state", "foo", "any", false),
+ ?assertMatch({Pid, _, ["foo"]}, wait_reply(Pid))
+ end).
+should_pass_correct_state_to_handle_config_stop({Pid, _}) ->
--- End diff --
These function needs a space between since they don't share the name and arity.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72265130
Agreed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72261403
All great!
@iilyak how do you think: if there need to use `config:get/3` with explicit default `undefined` value when the same value is used implicitly when you call `config:get/2`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-config pull request: 2561 make config api consistent
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/2#issuecomment-72220501
@iilyak great! thank you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---