Robotics Systems Types

Generally, the types look fine. There are, however, a few (few more ;) ) minor things I'd like to change. Don't be afraid. The list length corresponds to the patch length ;)

• I think the most persistent way to link an RFC is from the IETF website itself: http://www.ietf.org/rfc/rfc2445.txt. Maybe we can add this to the comments, too?
• Rename Usertype to UserType. These are two words and they should be reflected by the camel case usage.
• The comment of Usertype isn't clear to me in the conext of an Attendee. Could you please clarify what this mean?
• Please indent the asterisks for comment blocks correctly. All asterisks should form a vertical line.
• For comments, single newlines probably do not have a meaning comparable to tex etc. So something like:
  

      /**
      * Specifies the attendees calendar user address as URI.
      * This can for example be a MAILTO
      * corresponds to CAL-ADDRESS
      */
    

  
  will probably be rendered as: "Specifies the attendees calendar user address as URI. This can for example be a MAILTO corresponds to CAL-ADDRESS" in the generated RST documentation page. Please, either use empty lines to form paragraphs or real English sentences to fix this.
• I wouldn't mind if the words were complete in Attendee::Role. So REQ_PARTICIPANT could be REQUIRED_PARTICIPANT. This would be much easier to read even without the documentation. Are these fields modeled along the iCal specification and names are taken from there? Otherwise, I'd vote for the full names.
• Why is Attendee::member a comma-separated string? To me, a repeated field with the name groups sounds much clearer.
• I'd vote for cutype -> user_type to be clearer without strange abbreviations.
• part_status -> participation_status
• sentby -> sent_by
• What is meaning and format for directory?
• How is language language filled? ISO abbreviations or which format?
• For CalendarComponents:
  • Inside the same package it is usually not necessary to use the package for member types. Any reason why you did this?
  • Please remove the commented fields. We can add them with a new patch once they are implemented
• for DateTime:
  • Could this be part of rst/timing instead? I am not sure about this.
  • Comment for Type looks wrong.
  • What are the possible values for timeyone_id? Please specify this in the comment.
  • The comment for time with the explicit UTC sounds contradictory to the fact that also local times can be specified.
• I am not sure about the way duration is modeled. So far, we always tried to model times as timestamps in microseconds. Now we get a second kind of structure. Can someone please comment on this, too? As a first step to get an initial commit: Can you please add a TODO comment in the type header so that we remember to discuss this issue again?
• for Event:
  • Typo in the class comment: "an calendar" -> "a calendar"
  • Viewability: Is this a fixed term in iCal? Otherwise, Visibility sounds clearer to me.
  • Typo in Viewability::CONFIDENTIAL: "The event entry can be is confidential"
  • Please rename seq to sequence_number.
  • Rename transp to transparency
  • is uid really optional?
  • Typo in comment for url
  • Can we rename the various dt* variables to something more readable or are these names taken from iCal?
  • The two repeated fields categories and comment are inconsistent in the naming. We usually use these in plural form all the time.
  • Is there an ordering for comments? Is the first entry the most recent or oldest?
  • Please rename rstatus to request_status for readability.
• Organizer:
  • sentby -> sent_by
• RecurrenceId
  • RANGE: Please rename constants to reflect word boundaries, e.g. THIS_AND_XXX.
• RecurrenceRule
  • Typo "frequence" needs to be "frequency" in various positions of the file.
• Relationship::reltype sounds like a hard to understand name for the variable? Should we follow the weird iCal name or be more precise here?
• Todo::Viewability is a duplication from Event. Please factor this out into a new file.
• Todo in general: a lot of field are duplicated between Todo and Event. Would it make sense to collect these in a common type?
• Generally, there are several double spaces in the files. I will update the checker to detect these eventually, but could you please remove them.

Can you provide an updated patch that incorporates these proposals. Afterwards, we can merge this?

Viktor Richter wrote:

• Is there an ordering for comments? Is the first entry the most recent or oldest?

That is a good question. The standard does not provide any information about order in this part and neither google nor sogo nor thunderbird seem to support comments in events/todos.

Ok, so we'll leave it underspecified ;)

• for DateTime:
  • Could this be part of rst/timing instead? I am not sure about this.

Moving DateTime to timing would require to move TimeZone too because they are closely coupled. The DateTime (in RFC2445) is always needs an additional TimeZone definition when it is of type LOCAL. I think this is because DateTime is designed to be used for calendar purposes and not for (how i would understand) timing.

• The comment for time with the explicit UTC sounds contradictory to the fact that also local times can be specified.

The only other way here would be to define an extra DateTime which always is in UTC.

Option: DateTime could be split into a simple utc DateTime in timing (with time in milliseconds as its only field) and encapsulated by a DateTime in calendar, adding the needed timezone.

I think we're talking about different things here. What confuses me is the fact that the only real numeric value I can specify in DateTime is the filed milliseconds_since_epoch in your new patch. The comment for this field explicitly states that it should be provided in UTC, which is a timezone. This is contradictory to the fact that the timezone can also be set to LOCAL or FLOATING, which can only accidentally be in UTC but most likely in some other timezone.

So shouldn't the comment be that milliseconds_since_epoch is a unix time stamp starting from 1.1.1970 in the timezone specified in date_time_type field?

How about:

• timing/UtcTime with only milliseconds since epoch
• calendar/DateTime with UtcTime instead of milliseconds since epoch
• change all DateTime which require to be Utc to UtcDateTime

I don't think that is necessary once we get rid of the misunderstanding ;)

The patch is applied.

I still found two typos in variable and enum names. So you probably have to change your library, too, because I fixed these names in the patch.