-
Notifications
You must be signed in to change notification settings - Fork 66
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
Bag: Improve access #1602
Bag: Improve access #1602
Conversation
I'm a bit lost here. What/how is the |
payload is a json |
if path == 'root': | ||
current_data = payload | ||
else: | ||
dpath.new(current_data, path, payload, separator=".") | ||
write_db(current_data) |
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.
So.. Here we are replacing the entire db without checking if its structure is correct. I can't evaluate the implications because I'm not aware of the rest of the code, I believe in your judgment
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.
like.. a write_data("/root", {})
call would erase the db
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.
like.. a
write_data("/root", {})
call would erase the db
That's the point
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 have created an endpoint to only do that, making it more explicit
c001ed8
to
7b059c0
Compare
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
a82df28
to
be91908
Compare
be91908
to
41606db
Compare
core/services/bag_of_holding/main.py
Outdated
current_data = read_db() | ||
try: | ||
result = dpath.get(current_data, path, separator=".") | ||
result = dpath.util.search(current_data, path) |
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 not sure I like this, what is the purpose of using search? I think if we have something like
{
"potato: {
"a": 1,
"b": 2
}
}
and we ask for "potato/a" we should just get 1
, instead I'm getting:
{
"potato: {
"a": 1
}
}
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.
41606db
to
81e3727
Compare
- Use '/' as separator - Allow usage of wildcard * to get everything - Create an endpoint to allow overwriting db Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
81e3727
to
3f212ba
Compare
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.
If write_data()
is supposed to overwrite when the path already exists, this PR seems fine.
No description provided.