review pass on tutorial sections #806

fricklerhandwerk · 2023-11-22T03:32:10Z

it's a huge diff, but it doesn't change that much:

start with a mkDerivation scaffold that uses src = ./.
- this saves the detour with an intentionally broken derivation and
  the explanation it requires
- it provides an opportunity to drop another bit of information about
  toSource, namely that it always creates a directory in the store
use two source files to start with, otherwise the "try one file, then
the whole directory" approach is not well-motivated
- this obviates spending words on justifying a contrived example,
  it flows more naturally
mostly use diffs to highlight the changes
- this is not a strong proposal
- the explicit-include-union example starts over, so the build-up is
  manageable
- it works out quite nicely overall: since there is sufficient repetition
  where we reasonably expect learners to have gotten the point,
  the last two sections can be reduced just to the relevant bit.
rename section headers to match functions, not domain problems
- one exception is the split of the two union approaches, which should
  be okay
reword all instructions to be imperative

the review made clear how powerful and elegant the library is, and that the
tutorial is utterly well-structured and ordered in a way that feels natural
and covers all important aspects!

great job @infinisil!

- link to reference documentation on the `builder` executable - since the sandbox is enabled by default, we can just say that

- sharpen the value proposition of the library - this meshes with and expands on the new title

since the title is now different, we can be more specific on what the first section is about: file sets, generally.

brackets in console output invoke weird associations, as they could be syntactically relevant. bare ellipses are unusual enough one can expect them to be intuitively parsed as artificial.

three highlight boxes in a row seem a bit too much, also the tip is relevant immediately after the first example.

- more telling section title - use imperative in instructions

this follows the convention established in the FAQ on releases

this follows the convention established in other tutorials. also arguably, this file is *not* the package per se, but simply contains a representation of build instructions.

otherwise the examples don't actually work

- readers may not have literally seen the examples - "combinators" isn't even used in reference documentation and is a potentially scary word. it doesn't even convey additional meaning here, since combination ("two together") and composition ("put together") are almost the same, composition being more general.

it's a huge diff, but it doesn't change that much: - start with a `mkDerivation` scaffold that uses `src = ./.` - this saves the detour with an intentionally broken derivation and the explanation it requires - it provides an opportunity to drop another bit of information about `toSource`, namely that it always creates a directory in the store - use two source files to start with, otherwise the "try one file, then the whole directory" approach is not well-motivated - this obviates spending words on justifying a contrived example, it flows more naturally - mostly use diffs to highlight the changes - this is not a strong proposal - the explicit-include-union example starts over, so the build-up is manageable - it works out quite nicely overall: since there is sufficient repetition where we reasonably expect learners to have gotten the point, the last two sections can be reduced just to the relevant bit. - rename section headers to match functions, not domain problems - one exception is the split of the two union approaches, which should be okay - reword all instructions to be imperative the review made clear how powerful and elegant the library is, and that the tutorial is utterly well-structured and ordered in a way that feels natural and covers all important aspects! great job!

fricklerhandwerk · 2023-11-22T03:35:46Z

Nah, please someone tell that Vale thing to shut up. I'm really done for today.

infinisil

Awesome, thanks a lot! Here are some minor counter-suggestions :P

source/tutorials/file-sets.md

infinisil · 2023-11-22T03:50:12Z

source/tutorials/file-sets.md

-  sources ? import ./nix/sources.nix,
+  inputs ? import ./nix/sources.nix,


You used sources in the pinning tutorial, which I mirrored: https://nix.dev/guides/recipes/dependency-management 😄

I'm fine either way though :)

Ah, oh. Apparently I refrained from actually committing to that, but it stuck after spending a bit of time thinking it through. Now I remember: I didn't pull through because it would diverge from niv upstream docs and likely confuse people. Sorry for the detour.

(I still think we should consider converging with flake conventions, inputs or sources, either way. sources has some appeal because this is what the references are about: remote sources of... stuff. inputs is so generic to the point it's meaningless; but maybe that's good? At least they aren't even necessarily used as build inputs. Anyway, doesn't matter here.)

source/tutorials/file-sets.md

Co-authored-by: Silvan Mosberger <github@infinisil.com>

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

fricklerhandwerk · 2023-11-22T04:27:55Z

Suggestion stand-off! Feel free to merge what you like, it's all yours after all.

infinisil · 2023-11-22T04:31:28Z

Nice, thanks! Let's merge this one into #802 now :)

fricklerhandwerk added 21 commits November 22, 2023 04:26

more informative title

17e859e

shorten introduction

285fd17

- link to reference documentation on the `builder` executable - since the sandbox is enabled by default, we can just say that

clarify wording

6a61be9

reword learning objective

7e7c6d7

- sharpen the value proposition of the library - this meshes with and expands on the new title

Basics -> File sets

8402ecf

since the title is now different, we can be more specific on what the first section is about: file sets, generally.

clarify wording

06caaae

add link to command reference

d388d6d

shorten introduction to trace

76905ab

use log ellipsis as in the other tutorials

9fd9700

brackets in console output invoke weird associations, as they could be syntactically relevant. bare ellipses are unusual enough one can expect them to be intuitively parsed as artificial.

shorten introduction to path arguments

5ca3739

move tip before the other notes

568925e

three highlight boxes in a row seem a bit too much, also the tip is relevant immediately after the first example.

restructure notes on copying files

97bbe4f

add link to flakes documentation, add emphasis

fb6f630

trim down phrasing

767155b

A local directory -> Example project

152953e

- more telling section title - use imperative in instructions

channel -> channel branch

51a78a0

this follows the convention established in the FAQ on releases

package.nix -> build.nix

a6e5f26

this follows the convention established in other tutorials. also arguably, this file is *not* the package per se, but simply contains a representation of build instructions.

add instruction to add a source file

501a5f7

otherwise the examples don't actually work

follow convention from niv guide

ba4989c

fricklerhandwerk requested a review from a team as a code owner November 22, 2023 03:32

infinisil requested changes Nov 22, 2023

View reviewed changes

infinisil reviewed Nov 22, 2023

View reviewed changes

source/tutorials/file-sets.md Outdated Show resolved Hide resolved

fricklerhandwerk and others added 3 commits November 22, 2023 05:19

fix omission

fa037e6

Co-authored-by: Silvan Mosberger <github@infinisil.com>

Update source/tutorials/file-sets.md

429bbb0

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

Update source/tutorials/file-sets.md

16db564

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

Apply suggestions from code review

993bbc1

infinisil merged commit 8e3032a into NixOS:source-file-selection Nov 22, 2023
4 of 5 checks passed

infinisil mentioned this pull request Nov 22, 2023

File sets tutorial #802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review pass on tutorial sections #806

review pass on tutorial sections #806

fricklerhandwerk commented Nov 22, 2023 •

edited

Loading

fricklerhandwerk commented Nov 22, 2023

infinisil left a comment

infinisil Nov 22, 2023 •

edited

Loading

fricklerhandwerk Nov 22, 2023 •

edited

Loading

fricklerhandwerk commented Nov 22, 2023 •

edited

Loading

infinisil commented Nov 22, 2023

		sources ? import ./nix/sources.nix,
		inputs ? import ./nix/sources.nix,

review pass on tutorial sections #806

review pass on tutorial sections #806

Conversation

fricklerhandwerk commented Nov 22, 2023 • edited Loading

fricklerhandwerk commented Nov 22, 2023

infinisil left a comment

Choose a reason for hiding this comment

infinisil Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

fricklerhandwerk Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

fricklerhandwerk commented Nov 22, 2023 • edited Loading

infinisil commented Nov 22, 2023

fricklerhandwerk commented Nov 22, 2023 •

edited

Loading

infinisil Nov 22, 2023 •

edited

Loading

fricklerhandwerk Nov 22, 2023 •

edited

Loading

fricklerhandwerk commented Nov 22, 2023 •

edited

Loading