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

Message.sequence: wording mistake? #926

Closed
omegacoleman opened this issue Jul 19, 2023 · 5 comments · Fixed by #938
Closed

Message.sequence: wording mistake? #926

omegacoleman opened this issue Jul 19, 2023 · 5 comments · Fixed by #938
Labels
bug Something isn't working

Comments

@omegacoleman
Copy link
Contributor

Description

Hi, I'm trying to implement a mcap-compatible bag facility for a self-driving system. While I appreciate your hard work, I suspect a wording mistake in spec/index.md:

### Message (op=0x05)

...

| 4     | sequence     | uint32    | Optional message counter assigned by publisher. If not assigned by publisher, must be recorded by the recorder. |

It says the sequence is optional, but "If not assigned by publisher, must be recorded by the recorder.", wouldn't that contradict with 'optional'?

It seems like a wording mistake of "If assigned by publisher, must be recorded by the recorder."

@omegacoleman omegacoleman added the bug Something isn't working label Jul 19, 2023
@foxhubber
Copy link

foxhubber bot commented Jul 19, 2023

Internal tracking ticket: FG-4352

@omegacoleman
Copy link
Contributor Author

I proposed a fix in #927

@jtbandes
Copy link
Member

Hi, I believe this is not really a wording mistake per se... it's trying to say that since the field is required, if the publisher does not provide a sequence number then the recorder will have to come up with one. However, I agree that the wording is unclear. We will discuss internally.

@omegacoleman
Copy link
Contributor Author

Hi, I believe this is not really a wording mistake per se... it's trying to say that since the field is required, if the publisher does not provide a sequence number then the recorder will have to come up with one.

Thanks for clarifying that out, this is important to me.

However, I agree that the wording is unclear. We will discuss internally.

I'll leave this open for a while, since that would still contradict with the Optional part.

My suggestions would be:

  • if the recorder MUST come up with a sequence, then the field is not optional, it's mandatory
  • Add the the recorder will have to come up with one part to the specs to let everybody know

@wkalt
Copy link
Contributor

wkalt commented Jul 28, 2023

@omegacoleman I have raised a PR here to propose some new wording if you're interested: #938

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
3 participants