Skip to content

Introduce machine-readable manifest diagnostics#3127

Closed
Arcticae wants to merge 1 commit into
mainfrom
arcticae/metadata-machine-diagnostics
Closed

Introduce machine-readable manifest diagnostics#3127
Arcticae wants to merge 1 commit into
mainfrom
arcticae/metadata-machine-diagnostics

Conversation

@Arcticae
Copy link
Copy Markdown
Member

No description provided.

@Arcticae Arcticae requested a review from a team as a code owner May 19, 2026 10:13
@Arcticae Arcticae requested review from maciektr and wawel37 and removed request for a team May 19, 2026 10:13
@Arcticae
Copy link
Copy Markdown
Member Author

Arcticae commented May 19, 2026

@Arcticae Arcticae force-pushed the arcticae/metadata-machine-diagnostics branch 4 times, most recently from fcffdb8 to e5a6b47 Compare May 19, 2026 12:11
@Arcticae Arcticae force-pushed the arcticae/metadata-machine-diagnostics branch from e5a6b47 to 91b68f5 Compare May 19, 2026 12:43
Copy link
Copy Markdown
Contributor

@maciektr maciektr left a comment

Choose a reason for hiding this comment

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

Looks good overall

})
.unwrap_or(MachineDiagnosticSpan { start: 0, end: 0 }),
);
diagnostic.related = related
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: maybe use a builder pattern instead? could get rid of setting the defaults too.

span,
related,
}));
config.ui().force_print(MachineMessage(diagnostic));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: could be just Message impl for MachineDiagnostic.

}
}

pub fn all_crate_inputs(db: &dyn Database) -> Vec<CrateInput> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where is this used?

start: span.start,
end: span.end,
})
.unwrap_or(MachineDiagnosticSpan { start: 0, end: 0 }),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be default for MachineDiagnosticSpan?

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.

+1, if the span is no longer an Option, the 0,0 span should be set as a default

}

#[derive(Debug, Clone, Serialize)]
pub struct MachineRelatedLocation {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
pub struct MachineRelatedLocation {
pub struct MachineDiagnosticRelatedLocation {

}

#[derive(Debug, Clone, Serialize)]
pub struct MachineDiagnosticData {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Copy Markdown
Member

@wawel37 wawel37 left a comment

Choose a reason for hiding this comment

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

Seems fine, some small nit picks only

start: span.start,
end: span.end,
})
.unwrap_or(MachineDiagnosticSpan { start: 0, end: 0 }),
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.

+1, if the span is no longer an Option, the 0,0 span should be set as a default

MachineDiagnosticKind::ManifestDiagnostic,
sem.to_string(),
MachineDiagnosticSeverity::Error,
file.unwrap_or_else(|| "<unknown>".to_string()),
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.

nit: i'd assign the <unknown> string to some consts and have it named properly, so its clear it describes "no file" state

@Arcticae Arcticae closed this May 25, 2026
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.

3 participants