Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add helpers for unmarshal JSON object into Options map #9

Closed
wants to merge 6 commits into from

Conversation

ZiroKyl
Copy link
Contributor

@ZiroKyl ZiroKyl commented Jan 7, 2015

Implements the json.Unmarshaler interface for DHCP Options map.
This adds simple way to init DHCP Options from JSON object. For example, read DHCP Options from JSON formatted config file.

P.S. go test also been implemented.

@krolaw
Copy link
Owner

krolaw commented Jan 7, 2015

Hi, IMHO I don't think that sort of functionality belongs in the dhcp4 package as it near doubles the source size, on the oft chance that the user wants to store DHCP options, in that particular JSON format.

However, I do think there is merit in having this code in a separate package, which imports the dhcp4 package for the Options type. That way those wanting the functionality can import your package along with my one.

Thoughts?

@ZiroKyl
Copy link
Contributor Author

ZiroKyl commented Jan 7, 2015

I like this idea (separate package or sub package) but Go doesn't allow to do this: How do I import Parent-package from Sub-package for method?:

This does not work, you may not define a method on a type you did not
declare in your own package, exported or not.

@krolaw
Copy link
Owner

krolaw commented Jan 7, 2015

Perhaps something like:
type JSONOptions dhcp4.Options

Change object from dhcp4.Options to JSONOptions:
func (o *JSONOptions) UnmarshalJSON(b []byte) error { ... }

And when finished (in the example) cast your JSONOptions to dhcp4.Options.

@ZiroKyl
Copy link
Contributor Author

ZiroKyl commented Jan 8, 2015

Done. Usage:

import "encoding/json"
import "github.com/krolaw/dhcp4"
import "github.com/krolaw/dhcp4/reflectDHCP"
...
var opt = dhcp4.Options{};
json.Unmarshal(strJSON, (*reflectDHCP.Options)(&opt));

@krolaw
Copy link
Owner

krolaw commented Jan 8, 2015

Not wanting to have misled, but I imagined the separate package would have been called something like:
github.com/ZiroKyl/JSONdhcp4

That way it's your feature, your responsibility. I won't need to sign off or check improvements AND I can redirect requests for XML, TOML, bind, etc formats to their own packages that I also don't manage :-)

ZiroKyl added a commit to ZiroKyl/reflectDHCP that referenced this pull request Jan 8, 2015
@ZiroKyl
Copy link
Contributor Author

ZiroKyl commented Jan 8, 2015

@ZiroKyl ZiroKyl closed this Jan 8, 2015
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