-
Notifications
You must be signed in to change notification settings - Fork 33
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
Refactoring Monitor type #30
Comments
monitors JSON decodingI implemented two decoding method and run benchmark.
diff: master...haya14busa:refactor-monitor-type-json
I suspected that method 1. might be slow because it needs additional unmarshaling JSON, but bench shows it's faster than method 2 (though both implementations are fast enough, i guess). So, I'll implement method 1 to handle decoding JSON. |
All main refactoring tasks are done! 🎉 As a follow up, we can remove needless But, I think this issue is closable. Removing |
Thank you for your great contribution! 👍 |
Problem
Currently,
type Monitor
represents all monitor types, so required fields for each monitor type are unclear.Also, JSON tag must have
omitempty
because of above reason.Solution
Define each monitor type and Monitor interface.
Users can use type switch to access each monitor type like below.
We also needs to handle decoding JSON to each monitors types.
I'm considering to handle this problem by executing unmrashaling twice (1. looking for "type" field and 2. decoding to the monitor type) or decoding to
map[string]interface{}
once and converting it to each monitor type with "type" value using mitchellh/mapstructure.Pull-Request plan
Possible additional actions after above works:
omitempty
tag in each monitor types.GetID()
which returns monitor id). Common keys of monitor JSON are id, name, type, isMute, and notificationInterval.The text was updated successfully, but these errors were encountered: