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

refac(examples): Denest the source #724

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

Conversation

goyalyashpal
Copy link

@goyalyashpal goyalyashpal commented Aug 2, 2024

Hi!

I was just trying to understand the disko's examples, and i did this refactoring.

i found this interesting and thought to share with yous and ask if you'd be interested to get a merge request refactoring others in a similar fashion too 😃?

image


i tried to run the nix-instantiate --eval --strict (nixel reference) and yes, these evaluated to absolutely same thing 😃

$ nix-instantiate --eval --strict legacy-table.nix
{ disko = { devices = { disk = { main = { content = { format = "gpt"; partitions = [ { bootable = true; content = { format = "vfat"; mountpoint = "/boot"; type = "filesystem"; }; end = "500M"; name = "ESP"; start = "1M"; } { bootable = true; content = { format = "ext4"; mountpoint = "/"; type = "filesystem"; }; end = "100%"; name = "root"; part-type = "primary"; start = "500M"; } ]; type = "table"; }; device = "/dev/sda"; type = "disk"; }; }; }; }; }

$ nix-instantiate --eval --strict legacy-table-refactored.nix 
{ disko = { devices = { disk = { main = { content = { format = "gpt"; partitions = [ { bootable = true; content = { format = "vfat"; mountpoint = "/boot"; type = "filesystem"; }; end = "500M"; name = "ESP"; start = "1M"; } { bootable = true; content = { format = "ext4"; mountpoint = "/"; type = "filesystem"; }; end = "100%"; name = "root"; part-type = "primary"; start = "500M"; } ]; type = "table"; }; device = "/dev/sda"; type = "disk"; }; }; }; }; }

a more elaborate setup to compare automated-ly, rather than eyeballing 👀 :

eval-diff() {
  # Show diff of strict-eval of 2 nix files

  # Get the parameters
  local oldfile=$1
  local newfile=$2

  # Function: Abstract out the flags in invocation
  sevalnix() { nix-instantiate --eval --strict $1; }

  # Strict-evaluate both the files
  local oldval="$(sevalnix ${oldfile})"
  local newval="$(sevalnix ${newfile})"

  # Use process substitution to make stdout act as files to `diff` utility
  diff -u <(echo "$oldval") <(echo "$newval") | 

  # Pipe to `bat` with syntax-highlighting for diff
  bat -l diff;
}
$ eval-diff legacy-table{,-refactored}.nix 
───────┬──────────────────────────────────────────────────────────────
       │ STDIN   <EMPTY>
───────┴──────────────────────────────────────────────────────────────

@Mic92 Mic92 requested a review from Lassulus August 12, 2024 04:20
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Looks easier to read to me.

@Lassulus
Copy link
Collaborator

not sure, this touches the legacy partitions which we should migrate from anyway. Also by splitting it up people could be inclined to split the code over multiple files and this will trigger weird errors.

@Mic92
Copy link
Member

Mic92 commented Aug 12, 2024

not sure, this touches the legacy partitions which we should migrate from anyway.

I think this was just meant as an example to show what this would look like for all examples.
Also as long as we still have uboot firmware that requires fat partitioning, we cannot get rid of it until there is a proper replacement for MBR.

Also by splitting it up people could be inclined to split the code over multiple files and this will trigger weird errors.

What parts don't merge correctly? There are ways to allow only one instance to be defined.
I think this is not a very strong argument to make things less readable for everyone.

@Lassulus
Copy link
Collaborator

What parts don't merge correctly? There are ways to allow only one instance to be defined. I think this is not a very strong argument to make things less readable for everyone.

#678

@goyalyashpal
Copy link
Author

goyalyashpal commented Aug 14, 2024

There are ways to allow only one instance to be defined.
- @ Mic92 at #724 (comment)

what ways? can those help in this #678, where same key was defined more than once?
it would be awesome if there is 😃

by splitting it up people could be inclined to split ... trigger[ing] weird errors [as seen in] #678
- @ Lassulus at #724 (comment)

how about some note like any of following or smth similar; maybe in the example files themselves?

  • "Do NOT do multiple-conflicting definitions for same key"
  • "Unexpected behaviour for multiple-conflicting definitions for same key"
  • "Do not define same key multiple times"
  • "Unexpected behaviour if same key defined more than once"
  • ...

'cz in the current form, the examples are unreadable and don't really fulfill their purpose to the full potent.

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.

3 participants