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

nix_rs: Don't hardcode flake schema types #235

Merged
merged 35 commits into from
Sep 24, 2024
Merged

nix_rs: Don't hardcode flake schema types #235

merged 35 commits into from
Sep 24, 2024

Conversation

shivaraj-bh
Copy link
Member

@shivaraj-bh shivaraj-bh commented Aug 26, 2024

resolves #262

TODO:

  • Everything about flake-schemas must be in schema.rs

@shivaraj-bh
Copy link
Member Author

I will also have to re-think this design based on how it will be used in #222, to parse om flake output.

@shivaraj-bh shivaraj-bh changed the title nix_rs: Remove schema module; Enable generic schemas lookup from FlakeOutputs nix_rs: Don't hardcode flake schema types Sep 19, 2024
@shivaraj-bh shivaraj-bh marked this pull request as ready for review September 19, 2024 19:53
@shivaraj-bh shivaraj-bh marked this pull request as draft September 21, 2024 03:47
@shivaraj-bh shivaraj-bh marked this pull request as ready for review September 23, 2024 19:28
srid added a commit that referenced this pull request Sep 23, 2024
- Use `#![warn(missing_docs)]` to enforce that all public entites are documented
- Make an exception for `schema.rs` & `output.rs` since those can be documented as part of #235
srid added a commit that referenced this pull request Sep 23, 2024
- Use `#![warn(missing_docs)]` to enforce that all public entites are documented
- Make an exception for `schema.rs` & `output.rs` since those can be documented as part of #235
srid added a commit that referenced this pull request Sep 23, 2024
- Use `#![warn(missing_docs)]` to enforce that all public entites are documented
- Make an exception for `schema.rs` & `output.rs` since those can be documented as part of #235
Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

Just reviewed. @shivaraj-bh Can you merge from main to bring in #283 (removing #![allow(missing_docs)] in the process) and document the new types/ functions accordingly?

(Bear in mind that documentation should be more about general functional semantics than verbatim repetition of types).

Comment on lines +181 to +182
#[serde(other)]
Unknown,
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should be Other(String)? Assuming flake-schemas passes the arbitrary type string, that is. Not sure.

Copy link
Member

Choose a reason for hiding this comment

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

Can address this in different PR. I'm merging this.

The "Option" is not really used by the function. And default handling is typically done by the caller. This function only cares to take FlakeOutput, not the meta struture.
Also rename the function, since 'children' is a superset of what this actually returns.
Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

Good stuff 👍🏿

@srid srid merged commit 9934e02 into main Sep 24, 2024
6 checks passed
@srid srid deleted the generic-schemas branch September 24, 2024 04:11
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.

Don't hardcode flake schema types
2 participants