Skip to content

Implement mta_format_metadata#253

Open
GardevoirX wants to merge 2 commits into
metatomic-corefrom
core-metadata-print
Open

Implement mta_format_metadata#253
GardevoirX wants to merge 2 commits into
metatomic-corefrom
core-metadata-print

Conversation

@GardevoirX
Copy link
Copy Markdown
Contributor

@GardevoirX GardevoirX commented Jun 1, 2026

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

Reviewer checklist

  • CHANGELOG updated with public API or any other important changes?

📚 Download documentation for this pull-request

@GardevoirX GardevoirX requested a review from Luthaf June 1, 2026 11:55
@GardevoirX GardevoirX force-pushed the core-metadata-print branch from 6c502e1 to b40009c Compare June 1, 2026 12:01
Comment thread metatomic-core/src/metadata.rs Outdated
fn validate(&self) {
for author in &self.authors {
if author.is_empty() {
panic!("author can not be empty string in ModelMetadata");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not panic, instead return Result::Err

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please do validation during deserialization, and not as a separate step.

To make the models authors live easier, I might make sense to skip empty values instead of erroring, but no strong feeling on this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm throwing an error for empty values is our current behavior

for (const auto& author: this->authors) {
if (author.empty()) {
C10_THROW_ERROR(ValueError, "author can not be empty string in ModelMetadata");
}
}

@GardevoirX GardevoirX requested a review from Luthaf June 1, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants