Skip to content
This repository has been archived by the owner on Nov 30, 2023. It is now read-only.

add and remove methods added #42

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

halilkaya
Copy link

@halilkaya halilkaya commented May 30, 2016

With this PR, 2 methods which are add and remove added to Kaptan.

  • add: Adds a key-value pair to the current data.

Current data: {'product': {'price': 1.25, 'currencies': ['TL', 'EUR']}}

with .add('product.isPaid', True) it changes to: {'product': {'price': 1.25, 'currencies': ['TL', 'EUR'], 'isPaid': true}}

with .add('product.currencies', 'USD') it changes to: {'product': {'price': 1.25, 'currencies': ['TL', 'EUR', 'USD']}}

with .add('product.price', 22) it raises error like Key price already exist

with .add('product.price', 22, True) it replaces price value. So, last parameter is to replace.

  • remove: Removes a key from the current data.

with .remove('product.currencies') it changes to: {'product': {'price': 1.25}}

with .remove('product.currencies', 0) it changes to: {'product': {'price': 1.25, 'currencies': ['EUR']}}. If last parameter given, it refers index of an array.

@@ -125,6 +125,56 @@ def get(self, key=None, default=SENTINEL):
return default
raise

def add(self, key=None, value=None, replace=False):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why key field may be null ?

I think, you should remove the following code

if not key:
    raise RuntimeError("Unable to find key")

and key field must be mandatory.

@halilkaya
Copy link
Author

halilkaya commented Jun 3, 2016

Thanks @bahattincinic

@tony
Copy link
Collaborator

tony commented Jun 3, 2016

I've done JS so I'm familiar with using dot notation to access data. I've tried to take a lot of my JS patterns to python over the years and, to my surprise, ended up regretting it.

I could imagine being thrilled about adding a feature like this to kaptan 2 years ago or so.

Knowing what I know now, I don't really know if kaptan should be making an opinion on accessing nested data via dot notation, or adding and removing things in this way.

I think a better approach would be to make it more convenient / obvious to access the underlying dictionary data upon import. That makes it possible to use something like:

The other option would be to add a class argument when importing to cast the incoming dict as a SymbolDict/Bunch/DotMap object. This would ensure equivalent behavior while not duplicating behavior other dictionary wrappers do.

That's my opinion, Agree? Disagree? Thoughts?

@halilkaya
Copy link
Author

You are right, actually. I though it first, but didn't want to change the structure.

We can use EasyDict to use data with dot notation in Kaptan.

If it's OK to use EasyDict, I can refactor the code.

@tony
Copy link
Collaborator

tony commented Jun 8, 2016

@halilkaya sure, any of them. You can create a common interface between all of them I think, since they all use dict.

Can you do it in a way that makes the dependency optional?

@halilkaya
Copy link
Author

@tony sure. It had better be optional for backward compatibility. I'll do that. :)

@halilkaya
Copy link
Author

@tony sorry for not dealing with this for a while. I've searched a little bit about using dot notation in Python dicts. But all the results suggest to use default call method. So, we shouldn't use dot notation on calling parameters like JS. What do you think @bahattincinic @emre ?

@tony
Copy link
Collaborator

tony commented Oct 10, 2016

In python the dot parameters are typically for method calls, module namespacing, etc.

i went through that phase of wanting to write python like JS or ruby, in fact my earliest issues on this project were to make behave more like nconf. #10

then like yourself, I read more python code and fell into the best practices seen in PEP's and larger projects

@tony
Copy link
Collaborator

tony commented Aug 27, 2023

@halilkaya A significant amount of time has passed since this contribution was made - firstly, thank you for it!

Do you still want to pursue this change? If not, it's find to close.

I have not been in contact with the maintainer of the project in a few years.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants