Usages of `PackageName::unchecked` / How should errors look to users if we dont have a package name to display?

[mara-schulke]

          Having this means we can violate "type safety". Are there multiple places where we are using this?

I think I only saw one use of it, where we do: PackageName::unchecked("."). If that is the only use, do you think it might be a good strategy to implement Default instead and then call PackageName::default() there? 👵🏼

Originally posted by @xfbs in https://github.com/helsing-ai/buffrs/pull/194#discussion_r1422083467

[vsiles]

It looks like the only call to PackageName::unchecked could be replaced with a panic!, because if there is no file to read, Manifest::read() will return an Error and command::install will thread this error up the call stack.

If we want to be more resilient with future changes, we can also use a proper error instead of a panic.

[mara-schulke]

There is a no-panic, no-unwrap and no-expect policy (unwrittenly, I should condense this into a contribution guide / document.)

Can maybe we can find a rust idiomatic way of achieving the same?

[vsiles]

I'll give it some more thoughts. The "quick" fix would be to return a proper Err, which will never be returned in practice. But we might be able to tune the type definitions to ensure we have a proper path. I'll get some more time on it.

[vsiles]

My bad. I read it more carefully this week-end, and if the manifest doesn't have a package entry, this can totally happen. However if the long term plan is to get rid of the package entry, we can just wait and GC it when we get there.

[Leopard2A5]

I'm new to the code base, but two things occurred to me:

• is it actually valid/legal for a manifest to not have a proper [package] section? If it's not legal, returning an Err would be the cleanest solution.
• when DependencyGraph::from_manifest can't get the package name fron the manifest, it (imho) abuses the trust set in it by PackageName::unchecked to construct an invalid PackageName ("." is not valid when you ask PackageName::validate)