clugg / sm-json

A pure SourcePawn JSON encoder/decoder.
GNU General Public License v3.0
82 stars 8 forks source link

Default Values #11

Closed MAGNAT2645 closed 4 years ago

MAGNAT2645 commented 4 years ago

Would be good to implement default values to most getter functions like JSON_Object.GetInt (GetBool and etc.)

Code Snippet №1 (this might be better than 2nd):

methodmap JSON_Object < StringMap {
    public int GetInt(const char[] key, int defValue=-1) {
        int iValue;
        if ( !this.GetValue( key, iValue ) )
            iValue = defValue;

        return iValue;
    }
}

Code Snippet №2:

methodmap JSON_Object < StringMap {
    public int GetInt(const char[] key, int defValue=-1) {
        int iValue = defValue;
        this.GetValue( key, iValue ); // if key doesn't exist, iValue won't be overwritten.
        return iValue;
    }
}

So there won't be need to check for key existence.

clugg commented 4 years ago

Cool idea - all the object getters actually already return default values when a key is missing, but you can't override them by a param. Array getters have built-in key existence checking in GetIndexAsString, but ultimately it returns a default value anyway. I think adding a param for this is a good idea, will aim for the next release.

MAGNAT2645 commented 4 years ago

There's also one thing (idk if i should add it as another issue), when you use functions like json_merge would be good to have autoclosing option (like json_merge(JSON_Object to, JSON_Object from, bool replace = true, bool autoclose = false) so if object to already has object-typed keys, they'll be closed (via .Cleanup + .Close maybe) before being overwritten (when replace = true).

clugg commented 4 years ago

My only concern with automatic garbage collection in json_merge is the fact that we are not tracking instance references. If you happen to have an instance with more than one reference which is replaced during a merge then all other references to that instance will be rendered invalid. This same issue is mentioned in the "Cleaning Up" section of the README, the only difference being that Cleanup() is called manually and so the issue can be anticipated. I don't like the idea of introducing a "side effect" to merging, but if it is well documented and has a sensible default (probably autocleanup = false) then I suppose it's not a bad thing to have. I will hopefully have a release with these features tagged in the next few hours.

clugg commented 4 years ago

I have added support for both specifying default values and autoclosing objects on merge in v2.3.0. Please let me know if you have any issues. Thanks again!