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

Add Mcap Appender to the Typescript Core package #1012

Closed
wants to merge 3 commits into from

Conversation

alexern14
Copy link
Contributor

Public-Facing Changes

  • Added data end offset information to Typescipt Mcap Indexed Reader.
  • Added an Mcap Appender to the Typescript Mcap core package.

Description

  • Added data end offset information to Typescipt Mcap Indexed Reader for Mcap appending purposes.
  • Added an Mcap Appender to the Typescript Mcap core package in order to append metadata and attachments to existing Mcap files via the @mcap/core package.

@jtbandes
Copy link
Member

Thanks for the PR, this is interesting functionality. Can I ask why you chose to make this a separate class and not a feature of the existing writer class? (e.g. "open in append mode")

@alexern14
Copy link
Contributor Author

@jtbandes I originally started this by just changing the functionality of the writer class to be able to append, and while doing some cleanup I just created a separate class and was not quite sure how you guys would want this organized. I am open and willing to changing it to be additional functionality on the writer class though!

@jtbandes
Copy link
Member

I'm not sure how easily it would slot into the existing class but I do think, from the standpoint of a user, it would be more natural to be able to use the same writer class for either creating a new file or modifying an existing one. I think it could be worth exploring this (and maybe opening a separate PR if you think it's not clear which is better).

I also think the user would rather not have to do things like setStatistics / addChunkIndex explicitly, but instead the writer/appender should populate itself by reading the file during creation. This might necessitate an async create function, like we have for the indexed reader:

static async Initialize({

@alexern14
Copy link
Contributor Author

@jtbandes Thanks for the feedback! I just put up a different PR with that proposed implementation here #1016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants