-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Pull 66 add todo journal #84
Conversation
- Add methods for VTodo and VJournal - Changed the shared properties to be attached to the ComponentBase so those methods don't need to be repeated across all attributes that share them - If a property is shared across some, but not all components, then it's a private method on the ComponentBase and public method on the Components (eg LOCATION for Event and Todo but not Journal) - Shifted position of some Event methods in the file to live after the VEvent struct
- Add methods for Alarm, Timezone, and FreeBusy - Only methods for creating and adding right now, will add methods for the properties unique to them later
- Recursive call for Alarms - SetGeo was not a public method - Missing comma in struct creation
… into pull-66-add-todo-journal * origin/pull-66-add-todo-journal-orphan: Add changes for All Day Events to All Day Todos Add Comment and Category Properties to Components Fix Typos in a couple places Add Alarm, Timezone, and FreeBusy Property Methods Add Todo and Journal Property Methods # Conflicts: # calendar.go # components.go
Lint saved the day.. Gaps in testing by the looks of it. |
I'm reading thru it now to make sure everything looks good and catch any new functions that wouldn't have been originally changed. |
@spslater Thanks. There were a couple of relocations which made the merge a bit harder. If I had more time to work on this there is a lot I would like to improve. |
For sure, I should've really split it into 3 pull requests to make reviewing it a lot easier. I'll go in and do the official review. Later I will open a new pull request for any newer functions / ones that I missed. It'll be easier to catch them and make those changes that way. But everything I originally changed looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The merge looks good. Like I just said (I forgot there was a comment box here), I'll make a new pull request for any cosmetic things that don't line up. The newer added {Get,Set}LastModifiedAt
functions will still work with events, but won't with anything else.
@spslater Will review when they come through. I will wait until all your PRs are though before tagging. |
No description provided.