refactor: fix some readability nits #105
Conversation
components.go
Outdated
| @@ -808,14 +801,14 @@ type VAlarm struct { | |||
| ComponentBase | |||
| } | |||
|
|
|||
| func (c *VAlarm) Serialize() string { | |||
| func (alarm *VAlarm) Serialize() string { | |||
There was a problem hiding this comment.
In some cases your corrections have used an acronym (more than 2 words I gather) and in others the words. -- Intentional?
There was a problem hiding this comment.
I reworked it a bit. Using very short receiver names now, as is common practice. c for component, p for property. Then cb and bp up in their bases. And some other things.
There was a problem hiding this comment.
It's all good. I think we can start to develop a style guide from your work here. I like what you're doing though. It's good work! Thanks
However cal instead of c or calendar strikes me as nicer for the single word types.
|
I have no legitimate reason to reject this but can I argue for keeping the type switches? |
quite
force-pushed
the
readability-nits
branch
from
723233e
to
a2611c0
Compare
quite
force-pushed
the
readability-nits
branch
from
a2611c0
to
30de597
Compare
quite
force-pushed
the
readability-nits
branch
from
30de597
to
176f16f
Compare
| func (attendee *Attendee) Email() string { | ||
| if strings.HasPrefix(attendee.Value, "mailto:") { | ||
| return attendee.Value[len("mailto:"):] | ||
| func (p *Attendee) Email() string { |
There was a problem hiding this comment.
I wasn't pushing back on this. I was ensuring consistency / intent.
| func (calendar *Calendar) RemoveEvent(id string) { | ||
| for i := range calendar.Components { | ||
| switch event := calendar.Components[i].(type) { | ||
| func (cal *Calendar) RemoveEvent(id string) { |
There was a problem hiding this comment.
But I do like cal instead of c or calendar
There was a problem hiding this comment.
LGTM but I wasn't pushing back on the receiver rename just verifying / opening a discussion. However seen that calendar was cal not c makes me prefer that for single word types. Sorry, once again I will wait for your communication. If you want to talk to me about this I have opened up a discord just message me.
Sorry if it felt like I was pushing back.
|
No worries. I think it all turned out for the better now. Receiver names short, consistent to the point, and hopefully not causing confusion. |
| case *VAlarm: | ||
| r = append(r, alarm) | ||
| } | ||
| } | ||
| return r | ||
| } | ||
|
|
||
| func (alarm *VAlarm) SetAction(a Action, params ...PropertyParameter) { | ||
| alarm.SetProperty(ComponentPropertyAction, string(a), params...) | ||
| func (c *VAlarm) SetAction(a Action, params ...PropertyParameter) { |
There was a problem hiding this comment.
I guess at some point I should refactor this to va
No description provided.