EJSON support#109
Conversation
|
Hi @palkan Could you review this PR? |
palkan
left a comment
palkan
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Please, see the comments.
|
|
||
| private | ||
|
|
||
| def rel_config_pathes |
There was a problem hiding this comment.
| def rel_config_pathes | |
| def rel_config_paths |
|
|
||
| # Configure default loaders | ||
| loaders.append :yml, Loaders::YAML | ||
| loaders.append :ejson, Loaders::EJSON |
There was a problem hiding this comment.
Can we move which check here? I mean, add ejson loader iff a ejson executable is present. This way, we can only check once (and not every time we use the loader).
There was a problem hiding this comment.
Yep, sure. It looks like a more performant solution
| ejson_parser.call(abs_path), | ||
| rel_path | ||
| ] | ||
| end.find { |el| el.itself[0] } |
There was a problem hiding this comment.
We should merge local and env-specific (or global) data. See, for example, YAML or credentials loader.
So, it could be either [local] + environment or [local] + global.
|
|
||
| trace!(:ejson, path: relative_config_path) do | ||
| config_hash.transform_keys do |key| | ||
| if key[0] == "_" |
There was a problem hiding this comment.
I think, it should be a parser's responsibility to deal with the keys format. The responsibility of the loader is to pick the right source file path(-s) and generate Anyway Config specific data (traces, etc.).
|
Hi @palkan ! I don't know, if you got notification from Github about my second review request. Just a small ping =) |
|
Thanks! Merged via #122. P.S. Can you please provide your name in Russian, so I can mark the task on CultOfMartians as done? (Sorry, it's not clear from transliteration). |
2 participants
What is the purpose of this pull request?
Implements EJSON support.
Closes #106
What changes did you make? (overview)
Is there anything you'd like reviewers to focus on?
Checklist