Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29356103: Issue 3943 - add support of configuration file

Created:
Oct. 6, 2016, 1:03 p.m. by sergei
Modified:
Dec. 21, 2017, 10:42 a.m.
Reviewers:
tschuster
Visibility:
Public.

Patch Set 1 #

Total comments: 2

Patch Set 2 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -33 lines) Patch
M README.md View 1 chunk +11 lines, -1 line 0 comments Download
A config.json.example View 1 chunk +5 lines, -0 lines 0 comments Download
M run.py View 1 1 chunk +72 lines, -32 lines 0 comments Download

Messages

Total messages: 4
sergei
Oct. 6, 2016, 1:10 p.m. (2016-10-06 13:10:05 UTC) #1
sergei
could you please review it?
Nov. 21, 2016, 2:12 p.m. (2016-11-21 14:12:01 UTC) #2
tschuster
LGTM. https://codereview.adblockplus.org/29356103/diff/29356104/run.py File run.py (right): https://codereview.adblockplus.org/29356103/diff/29356104/run.py#newcode101 run.py:101: value = cli_parameters[field] if cli_parameters[field] is not None ...
Nov. 21, 2016, 6:07 p.m. (2016-11-21 18:07:55 UTC) #3
sergei
Nov. 22, 2016, 9:23 a.m. (2016-11-22 09:23:31 UTC) #4
https://codereview.adblockplus.org/29356103/diff/29356104/run.py
File run.py (right):

https://codereview.adblockplus.org/29356103/diff/29356104/run.py#newcode101
run.py:101: value = cli_parameters[field] if cli_parameters[field] is not None
else config_value
On 2016/11/21 18:07:55, tschuster wrote:
> Can't you just write:
> value = cli_parameters[field] if cli_parameters[field] is not None else
> config_parameters[field]
Changed. Now lines are not so long and you are right, config_parameters.get is
better than our implementation. config_parameters[field] throws exception if
there is no such key.
> 
> We should document that CLI parameters take precedence.
It's already documented in README.md.

Powered by Google App Engine
This is Rietveld