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

GetValueByNamespace extended to support retrieving values from maps a… #33

Merged
merged 1 commit into from Aug 10, 2016
Merged

Conversation

ghost
Copy link

@ghost ghost commented Aug 1, 2016

…nd arrays

Changes made in this PR:

  • extended GetValueByNamespace
  • unit tests updated - added cases of retrieving values from arrays and maps.

return GetStructValueByNamespace(object, ns)
case reflect.Struct:
return GetStructValueByNamespace(object, ns)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding default case to this switch with some log

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@lmroz
Copy link
Contributor

lmroz commented Aug 2, 2016

I think this code cannot manage with cases when:

  • On some level of composition you have a map or slice
  • Value is a pointer to anything else than struct

Even if that gonna stay unimplemented at least you should leave //TODO or bug godoc comment.

…nd arrays

//TODO added to GetValueByNamespace
@ghost
Copy link
Author

ghost commented Aug 10, 2016

@lmroz Thanks for your review. This change is needed for intelsdi-x/snap-plugin-collector-facter#15 so for now I add //TODO notes as you've suggested. Would you mind creating an issue with exact description of cases that should be handled by GetValueByNamespace()?

@IzabellaRaulin
Copy link
Contributor

IzabellaRaulin commented Aug 10, 2016

LGTM besides what @lmroz mentioned.

The previous form of GetValueByNamespace also manage only a pointer to struct , so I agree that it can be managed as the next step - the issue #34 was created to address that.

@IzabellaRaulin IzabellaRaulin merged commit 925baff into intelsdi-x:master Aug 10, 2016
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