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 some internal documentation for flake support objects. #4080

Merged
merged 5 commits into from Oct 19, 2020

Conversation

kquick
Copy link
Contributor

@kquick kquick commented Sep 26, 2020

Please feel free to update any parts that are not correct or poorly stated, but hopefully this will help for comprehension and maintenance of the flake support.

@@ -12,10 +12,25 @@ class Store;

typedef std::string FlakeId;

// The FlakeRef represents a local nix store reference to a flake
Copy link
Member

Choose a reason for hiding this comment

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

The "local nix store reference" bit is confusing. Maybe something like "A flake reference specifies how to fetch a flake (e.g. from a Git repository). It is created from a URL-like syntax (e.g. 'github:NixOS/patchelf') or from an attrset representation (e.g. '{ type = "github"; owner = "NixOS"; repo = "patchelf"; }').".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that does sound clearer. I made a couple of additional adjustments to this description that occurred to me as I was working on it; let me know what you think of the updated version.

@@ -17,20 +17,41 @@ struct FlakeInput;

typedef std::map<FlakeId, FlakeInput> FlakeInputs;

// FlakeInput is the flake-level parsed form of the "input" entries in
Copy link
Member

Choose a reason for hiding this comment

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

Could you use /* ... */ style comments? We mostly use // for single-line comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I made this in its own commit with no other wording changes (although some fmt wrapping updates occurred); all wording changes are in different commits to make it easier to see those changes.

@kquick
Copy link
Contributor Author

kquick commented Sep 28, 2020

Thanks for the suggestions, @edolstra . I've made all the updates and this is ready for reviewing again.

@kquick
Copy link
Contributor Author

kquick commented Oct 5, 2020

@edolstra pinging you since it would great to get this merged before things diverged.

@edolstra edolstra merged commit 9635fb7 into NixOS:master Oct 19, 2020
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.

None yet

2 participants