set($value, null) should set value to null, not delete it

[hassankhan]

For cases where null is actually a value, shouldn't we just set it as that.

If a value must be removed, we can add another method. Only reason being, and I'm pretty sure a test case would show this, is that in JSON, null is a valid value.

[hassankhan]

I've given it some more thought, and I think adding a delete() method sort of goes against the spirit of this library. That said, if you can set values at runtime you should be able to delete them too.

I've looked through the code some more and found if a value is set to null then it invalidates the cache as well, that's probably something that'll have to change too. Also, a fair amount of code will be shared between set() and delete(), so might need to refactor some of the code into a separate method.

[noodlehaus]

Initially when I did this, I think I had the mindset that configs, once loaded, should be read-only. I can't remember my exact train of thoughts or the use cases I had in mind that lead to the code as it is before the latest changes.

I agree with the point you raised with null values in JSON configs.

[hassankhan]

Haha, I literally came to the same conclusion. That being said, the set() method exists, so why not a delete() method too? I've been trying to think of a way of doing it but not had much luck so far, any suggestions?

[noodlehaus]

After thinking about it, now I'm actually more inclined to make the loaded configs read-only again. It makes sense that configuration changes should be done in config files. Also, it'll simplify the class even further. So the class now just focuses on resolving paths inside the config, and returning values.

What do you think?

[hassankhan]

Yep, sounds great to me, shall I close the issue then? As much as I would prefer a delete() (for purposes of completion), it seems like overkill and I've wasted enough time on it anyway 😄