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

ValidPathInfo JSON format should use null not omit field #9995

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Feb 12, 2024

Motivation

Context

Blocked on #9994, tests will fail until this

A little progress on #10311

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@thufschmitt
Copy link
Member

Discussed during the Nix maintainers meeting on 2024-02-26.
Idea approved, but :

  • Should be mentioned in the release notes
  • Should be reflected in the json guidelines policy
  • @edolstra: Potentially breaking change, although probably not a big one

    • Also only happens in the new command-line
  • @thufschmitt: Do we care about having these null fields?

    • @roberth: Have a slightly different semantics, potentially interesting to distinguish
  • Idea approved

  • Should be mentioned in the release notes

  • Should be reflected in the json guidelines policy

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-28-nix-team-meeting-129/40499/1

@Ericson2314 Ericson2314 marked this pull request as ready for review April 5, 2024 17:27
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 5, 2024
@Ericson2314 Ericson2314 added this to the nix-command stabilisation milestone Apr 5, 2024
@Ericson2314 Ericson2314 marked this pull request as draft May 30, 2024 14:42
@Ericson2314 Ericson2314 marked this pull request as ready for review May 31, 2024 21:46
@github-actions github-actions bot added documentation contributor-experience Developer experience for Nix contributors labels May 31, 2024
@Ericson2314 Ericson2314 force-pushed the json-empty-sigs branch 3 times, most recently from 335184d to f2195ef Compare May 31, 2024 21:56
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Have only reviewed docs so far.

doc/manual/src/contributing/json-guideline.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/json-guideline.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/json-guideline.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/json-guideline.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/json-guideline.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts: we should recommend that consumers ignore any fields they don't know.
I also like the GraphQL-like idea of specifying which attrs to expect, so that the consumer can safely use the whole value without anything unexpected occurring in it.
However, this seems less relevant (and unlikely to see much adoption) in the CLI; rather applies more to a fetchTree meta return attr (#8940 (comment)), where use of the whole object is actually relevant for reproducibility. Or as a solution for in-sandbox path info JSONs that isn't quite as capable as versioning, but does prevent over-fetching.

doc/manual/rl-next/store-object-info.md Outdated Show resolved Hide resolved
doc/manual/rl-next/store-object-info.md Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the json-empty-sigs branch 2 times, most recently from 387a3a5 to 1d6c50a Compare June 1, 2024 16:08
doc/manual/rl-next/store-object-info.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/json-guideline.md Outdated Show resolved Hide resolved
doc/manual/src/protocols/json/store-object-info.md Outdated Show resolved Hide resolved
src/libstore/parsed-derivations.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Nice!

doc/manual/src/contributing/json-guideline.md Outdated Show resolved Hide resolved
doc/manual/src/contributing/json-guideline.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Hensing <[email protected]>
Co-authored-by: Robert Hensing <[email protected]>
@Ericson2314 Ericson2314 merged commit c6add88 into master Jun 3, 2024
13 of 15 checks passed
@Ericson2314 Ericson2314 deleted the json-empty-sigs branch June 3, 2024 12:58
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-06-03-nix-team-meeting-minutes-149/46582/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors documentation with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants