Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Config refactor #298
Config refactor #298
Changes from 30 commits
7e67e68
0efccb6
24952e0
7d62a23
e44a1b7
e0afae1
3ca6a5b
abdc0af
2995cd9
0770172
e7eb96a
7bc48ca
fc5619a
9109a46
653f566
984c5a5
b90ca00
18a5d33
42071b0
ec800c3
c2e79d4
b912be1
2cef067
39538d3
b5e8229
47e483a
66ad7da
f8e2c29
03e2e04
4fa68ad
1f04d5c
b82c60f
544b60c
228a0c4
4614d83
09a3b3d
ef68ab5
3ca025e
3772015
d21f3fe
7c73d5c
47eac17
7bb97c8
e0938a9
17be8bb
3d2aa7d
6635879
0063663
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking it might be best to move all the configuration options to the module docstring. This has several advantages:
Less redundancy --- configuration options only need to be specified in one place, and linked from everywhere else
We can use Sphinx glossary notation to add more details
It is user facing, whereas these docstrings aren't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added them to the module docstring :) Added the list of environment variables, let me know if you think they need more adjustments :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to define the kwargs as 'belonging' to the "api" section when the defaults/types are defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that be referring to the defaults used in
create_config
?One of the problems I came across was that if I use
create_config
andkwargs
here, then having defaults will result in an unexpected overwrite.That is
where
'hostname'
and'port'
were overwritten, whereas they were not defined inkwargs
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. This is because the
other_config
is replacing the default values 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest changing this to
create_config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a case where a more verbose name makes it slightly harder to parse. I would suggest
update_config
is sufficient here, as it is similar to thedict.update()
(and feels quite pythonic)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, is this method needed at all? Would
config.update(other_config)
work?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this such that there the
dict.update
method is being used :)One thing to note is that it cannot be replaced entirely. This is because assuming that not all options are defined in
kwargs
, calling onupdate
on the outer dictionary does not update the inner dictionary, but overwrites it (updating is not recursive for the nested structure):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be made more succinct using the
dict.update
method?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it so, however, will have to add certain validity check most probably, such that no problems can arise from updating with invalid configurations.