Skip to content

Conversation

@tlaseke
Copy link

@tlaseke tlaseke commented Dec 3, 2013

This attempts to check the json parser string for repeated keys which otherwise throws an error preventing a RestResponse.data object from being created.

@timhall
Copy link
Member

timhall commented Dec 3, 2013

Thanks @tlaseke! I'm really excited that you've contributed! This is a good catch and with just a few changes it's ready to merge:

  1. Remove Dim Key As String. I know you didn't put this in, I accidentally left it in previously, but it's confusing with this new behavior
  2. Explicitly declare currentKey (String) and currentItem (Variant)
  3. Remove commented out line. It's saved in the history if we want to go back to it, so it's safe to just rip it out
  4. Remove "//" and "The purpose of this fix it to", but leave rest of that comment as it is nice and descriptive

In other areas of Excel-REST (e.g. adding parameters) the default behavior is to add if it doesn't exist and overwrite if it does exist. I think this should be the behavior here, but based on your use case, it may make more sense to skip duplicate keys. Which works better for you? Either way that you decide, I think the resulting behavior should be stated explicitly such as the following:

If Not json_parseObject.Exists(Key) Then
    json_parseObject.Add Key:=currentKey, Item:=currentItem
Else
    '  Do not overwrite on duplicate keys
End If
' OR
If Not json_parseObject.Exists(Key) Then
    json_parseObject.Add Key:=currentKey, Item:=currentItem
Else
    json_parseObject(currentKey) = currentItem
End If

@tlaseke
Copy link
Author

tlaseke commented Dec 3, 2013

I agree with the overwrite behavior as default. Change as you will. I'm not a software developer. Just a middle-manager trying to automate some chores. The fork was just a convenient way to communicate. -tl

@timhall
Copy link
Member

timhall commented Dec 3, 2013

Ok, thanks @tlaseke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants