Skip to content
This repository has been archived by the owner on Mar 31, 2024. It is now read-only.

Add support for setting values and writing the config #9

Open
jarrodchesney opened this issue Nov 9, 2016 · 9 comments
Open

Add support for setting values and writing the config #9

jarrodchesney opened this issue Nov 9, 2016 · 9 comments

Comments

@jarrodchesney
Copy link

This works for me.

Synerty@7214f33

@pasztorpisti
Copy link
Owner

pasztorpisti commented Feb 21, 2017

Thanks for letting me know. To be honest I was thinking about adding write functionality at the time of implementing the library but that would have introduced a few problems and the correct solution requires a lot of extra work. E.g.: retaining comments, perhaps formatting. This is why I decided create reader-only library which does the job for 90+% of projects that don't want to manipulate the config programmatically.

Despite this I think write is a good idea. However in that case I'd structure the project differently. If write would be supported (correctly - e.g.: optionally retaining comments) then I'd implement the whole thing as a general purpose extended json library that consisted of the current JSONParser plus a new JSONWriter object (that similarly to the Parser, operates with events - perhaps implements the parserlistener interface - and outputs json text in response to events).

On top of the previously described JSONParser, the general purpose extended json library should provide a ParserListener implementation that builds a python object hierarchy in response to events emitted by JSONParser (to be able to use the lib instead of the standard json.load), plus it should provide a utility function that can take a python object hierarchy and convert it to events for the JSONWriter in order to write json text (to replace the standard json.dump).

How json-cfg comes to the picture? json-cfg would be simply another package that depends on the JSONParser and JSONWriter of the extended json library. json-cfg would provide another ParserListener implementation to build a config object tree (instead of primitive python obj tree) based on the events emitted by JSONParser. The loaded json object tree could also provide a save method which would be implemented by recursively walking the config obj tree and emitting events to a JSONWriter.

As you see this is quite a lot of work and I can't allocate free time for this in the near future. For this reason I'd keep json-cfg as it is. I thought people who want to modify their config and are fine reverting the json.dump can do that without the support of this library. E.g.:

cfg = jsoncfg.load('my.cfg')
py_obj_tree = cfg()
py_obj_tree['whatever'] = ['woof', 'meow']
json.dumps(py_obj_tree)

@jarrodchesney
Copy link
Author

In my implementation (https://github.com/Synerty/json-cfg-rw), i do use json.dumps.

However, i've made changes to json-cfg nodes so that they auto create objects and populate them with the default values when they are missing.

This way i can start reading empty config files, use the default configuration like normal, and write the default config back out. Usign json-cfg to write my default config files programatically.

This also works when I want to add new properties to the file.

Having a JSONWriter as you say would be very nice.

@pasztorpisti
Copy link
Owner

However, i've made changes to json-cfg nodes so that they auto create objects and populate them with the default values when they are missing.

This is a strategy that is very specific to your application. It mixes read and write into a relatively complex operation that isn't intuitive for the user: someone using the library (or a newcomer who reads the existing config loader code) probably doesn't expect the config data to be altered as a result of a query.

Another problem with this solution is that it works with dicts but I can easily imagine scenarios where it doesn't work in with lists (fixed length, variable length, etc...). Those situations can't be solved with a compound "query_or_set_default" operation - they require custom code (if config exists then do this, else do that).

Unfortunately for the above reasons I would be hesitant to add your solution to the core - I think it isn't simple and/or general purpose.

Supporting assignment would be relatively easy to add (at least with dicts, lists could cause problem here too) but I haven't added it due to the lack of json write support. Lists are problematic because if someone says cfg.mylist[2].item.value = 5 then mylist has to be created, and items at index 0 and 1 have to be filled (None would probably be a sensible filler). Not a huge problem but isn't necessarily intuitive... Another problem is that some people might want to assign python object hierarchies like {"x":[{"y":0}, 5], "w": {"a": []}} to config objects and they might expect each embedded list and dict etc to be automatically wrapped into config node objects (or maybe not)... It is difficult if not impossible to solve these problems in a simple and intuitive way.

@jarrodchesney
Copy link
Author

  1. It doesn't write the file unless the user tells it to
  2. The default data that it auto creates has no effect on what is returned to the user. What json-cfg does under the covers is of no concern to the user if it has no external effect. Whether it should or shouldn't create default values for the save could be an option.
  3. Yes, the arrays are complicated. There are only some ways to update it that work - but wouldn't you have this same problem with your design since it's still json-cfg?

@pasztorpisti
Copy link
Owner

pasztorpisti commented Feb 22, 2017

  1. I would add file write support to the library only after a proper write implementation (that retains comments, some of the formatting e.g..: wrapped/unwrapped lists/objects).
  2. I wouldn't add this level of complexity to a simple general purpose library. In my opinion such logic should reside outside of the library in another library or an application. As you've seen I'd add even the write functionality by splitting json-cfg into two libraries and keeping the extended json reader/writer separated from the actual json-cfg nodes. Separating solutions into several layers makes it easier to understand things and people who need only the core general purpose functionality don't have to include the high-level "narrow-purpose" libs (e.g.: someone would be able to use the general purpose extended json lib without the json-cfg thingy). json-cfg itself is more complex and more narrow-purpose than an extended json library. Your solution is another layer that contains even more narrow-purpose functionality (needed by less people) than json-cfg.
  3. I don't have a design for this, I've just written a "raw braindump" about the topic. Because of the lack of simple design I think it is bad idea put an implementation into the library.

A bit off topic, but another thing that isn't solid is that the with blocks of python are usually used to free allocated resources that have to be freed regardless of the outcome (fail/success) - a with block is very similar to a try/finally block in which the finally block is executed to release resources even in case of failure/exception inside the try block. Your solution uses with to save data. Do you really want to save data even if the middle of the with block fails with an exception? I think this isn't what people expect from a with block - it's better to keep save operations explicit. If someone wants to do this odd thing (saving at the end of the block in case of failure) then they can do that with a try-finally block (hopefully with some comments why they want to do that) but I wouldn't encourage doing this by adding with support to the library that performs this operation.

@jarrodchesney
Copy link
Author

I've chosen to use the with block for convenience, the user can choose this or they can call "save_config" them selves.

Remembering that I use it to update the config file as well, I want to ensure that the data given to the user matches what is in the config file, so reading defaults always writes it back. Here is how I use it.

https://github.com/Synerty/peek-platform/blob/master/peek_platform/file_config/PeekFileConfigOsMixin.py#L23

@pasztorpisti
Copy link
Owner

pasztorpisti commented Feb 22, 2017

When people read code they expect with statements to release resources. It is a bad idea to use language features to do different things than most other programmers do with them, especially if you don't gain much. The example you've linked (https://github.com/Synerty/peek-platform/blob/master/peek_platform/file_config/PeekFileConfigOsMixin.py#L23) would actually be easier to understand for a new programmer and wouldn't at all be complicated without the with. Without the with magic it would be obvious that a file is saved (instead of wrongly assuming that something is released) and the code wouldn't be buggy: because currently it saves the file even if an exception happens inside the with block. In my opinion in this example you've added abstraction without winning much. Your code will be easier to understand and read if you use language features the same way as most other people do.

People look at the dog, expect it to bark, and then they are surprised when the dog says 'meow'......

To be honest I can't complain with json-cfg:

In json-cfg I've used the __getattr__ and __call__ operators to do "fancy stuff" (query/fetch config values). This is not what people usually expect for example from a __call__. I didn't do this because I thought it was a very good idea. I did this because I had some other high level goals to achieve and this seemed to be the least fancy way to achieve those goals. However in return for modifying the "widely expected" behaviour of these operators I've at least gained some valuable things:

  • Querying the config in a simple way but still getting the error location in the config if an exception happens
  • In case of deeply nested config values (dict inside dict inside list ....) you don't have to write a lot of if-s to check whether the required keys/indexes are valid.
  • Simple way to define defaults and validators/mappers as call parameters while fetching config values.

As you see I've paid a price: I've modified the "popular" behaviour of some operators in unusual ways (that is a bad-bad thing). But at least in exchange for relatively valuable things. I still don't think it to be an extremely good tradeoff, but it might be an OK one.

@jarrodchesney
Copy link
Author

In my opinion in this example you've added abstraction without winning much.

You're very correct. I'll keep this in mind, and thank you for the clarity.

@pasztorpisti
Copy link
Owner

pasztorpisti commented Feb 22, 2017

I understand your motives. As a programmer it is very tempting to hack a hackable language. I've hacked the sh*t out of python, C++ operator overriding, template metaprogramming, etc...

It took me more than a decade to realise that in case of large codebases it is much easier to navigate and read the code if I don't have to dig deep to find out how some deeply hidden magic works. It is a best case scenario when you can at least find out by reading the code that there is hidden magic. The much worse scenario is when you can't even see it by reading the code. E.g.: your with implementation, or for example an overridden __add__ operator that doesn't behave as normally expected and does something fancy.

I've also learned that abstractions are super dangerous. If you aren't convinced that an abstraction is useful then don't introduce it. Writing a bit bloaty code or copy-pasted code is less expensive than introducing the wrong kind of abstraction (that will be very difficult to eliminate/cleanup/refactored compared to bloat or copy-paste). An abstraction first has to be understood, then people have to realise that it is the wrong abstraction, then a strategy is needed to remove it... In case of bloat and copy-paste: it is bloody obvious how to deal with it even if its ugly.

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

No branches or pull requests

2 participants