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 the package std module #2104

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add the package std module #2104

wants to merge 1 commit into from

Conversation

jneem
Copy link
Member

@jneem jneem commented Nov 22, 2024

Adds a package module to std, with contracts related to package manifests.

As I was making this PR, it occurred to me that maybe some of the contracts (e.g. the semver ones) could be parsing into a structured representation instead of just validating. What do you think?

@jneem jneem requested a review from yannham November 22, 2024 03:08
Copy link
Contributor

Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

Looks reasonable so far.

With respect to semantic versioning, we could indeed parse it on the way (since we're running the regex anyway), I guess that will spare re-parsing it on the Rust side. It would also allow people (if we write the right contract) to use either a structured representation or a textual representation as an input. The final representation would be always structured (in fact even that is not required, we could keep a union, but I don't think it has much of an advantage).

Regarding optionals, I don't have a strong opinion myself but I guess we could look at the manifest format of some prominent languages that seem to have done things right and try to extract a common base of attributes that are required everywhere.

Comment on lines +2931 to +2933
| doc m%"
The name of this package.
"%,
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: we indent the documentation in the rest of the stdlib.

Suggested change
| doc m%"
The name of this package.
"%,
| doc m%"
The name of this package.
"%,

core/stdlib/std.ncl Show resolved Hide resolved
"%,

keywords
| Array String
Copy link
Member

Choose a reason for hiding this comment

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

Should this be enum tags instead? But it's less nice to express as a static type (we can use a contract like std.enum.EnumTag or I don't remember how it's called, but for a static type we would need proper existential like exists a. [| ; a |])

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly I picked String because the set of reasonable values is open and possibly large.


# TODO: maybe restrict this to be a valid SPDX 2.3 license expression?
# Cargo allows anything here, but applies restrictions when trying to
# publish to crates.io.
Copy link
Member

Choose a reason for hiding this comment

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

Cargo convention sounds reasonable: if people are using their own registry or package locally, then we should probably not annoy them with such restrictions - but pushing to the registry might come with additional ones.

| {
_ : [|
'Path String,
'Git {
Copy link
Member

Choose a reason for hiding this comment

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

I was going to say that we should put each payload into its own type, but then that won't be usable in statically typed anymore as we don't have let types yet. Is that why you inlined all the structure here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, it was just because the first version had less fields, and then I didn't split it out as the number of fields grew. I'm not sure that static typing is a big concern here, as we don't have functions that consume and produce Manifests. (At least, not yet. But by the time we do, maybe we'll also have let type...)

Copy link
Member

Choose a reason for hiding this comment

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

Well, if we put it in the stdib, people might want to do that, so we could be tempted to leave it as it is and then later split it in a nicer way once we have let types, without disturbing user code.

On the other hand if we split it into contracts now this reduces what users can do with manifests but we can still introduce let-types later and that would be a backward-compatible improvement, which has been our strategy for many things up to now. So indeed if the stdlib doesn't really need the statically typed side, I guess we can go both ways.

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