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

Handle raw config for devshell & nixago #294

Open
blaggacao opened this issue Apr 24, 2023 · 6 comments
Open

Handle raw config for devshell & nixago #294

blaggacao opened this issue Apr 24, 2023 · 6 comments
Labels
👀 Status: Review Needed This issue is pending a review 💪 Effort: 3 This issue is of medium complexity or only partly well understood

Comments

@blaggacao
Copy link
Collaborator

Instead of mixing config intent and rendering / re-ification like this:

{
  bench = dev.mkShell {
    name = lib.mkDefault "Bench Shell";
    nixago = [
      (dev.mkNixago cell.config.procfile)
      (dev.mkNixago cell.config.redis_socketio)
      (dev.mkNixago cell.config.redis_queue)
      (dev.mkNixago cell.config.redis_cache)
    ];
  };
}

Keep only the intent (config), like so:

{
  bench = {
    name = lib.mkDefault "Bench Shell";
    nixago = [
      cell.config.procfile
      cell.config.redis_socketio
      cell.config.redis_queue
      cell.config.redis_cache
    ];
  };
}

The re-ification can be provided via block type actions.

This increases ease of composition for these configuration targets.

Caveat: harvesting from config won't be possible anymore. For harvesting, users would have to re-ify themselves in a separate block type.

@blaggacao blaggacao added 👀 Status: Review Needed This issue is pending a review 💪 Effort: 3 This issue is of medium complexity or only partly well understood labels Apr 24, 2023
@Pegasust
Copy link
Contributor

Not sure if this is the right issue here, but the latest commits in some way addressed this by changing:

{
  bench = dev.mkShell {
    name = lib.mkDefault "Bench Shell";
    nixago = [
      (dev.mkNixago (lib.cfg.procfile cell.config.raw_procfile))
      (dev.mkNixago (lib.cfg.redis cell.config.raw_redis_socketio))
    ];
  };
}

onto

{
  bench = dev.mkShell {
    name = lib.mkDefault "Bench Shell";
    nixago = [
      ((dev.mkNixago lib.cfg.procfile) cell.config.raw_procfile)
      ((dev.mkNixago lib.cfg.redis) cell.config.raw_redis_socketio)
    ];
  };
}

from what I observe in

std/src/local/shells.nix

Lines 12 to 24 in 1dfd574

nixago = [
((lib.dev.mkNixago lib.cfg.conform)
{data = {inherit (inputs) cells;};})
((lib.dev.mkNixago lib.cfg.treefmt)
cell.configs.treefmt)
((lib.dev.mkNixago lib.cfg.editorconfig)
cell.configs.editorconfig)
((lib.dev.mkNixago lib.cfg.githubsettings)
cell.configs.githubsettings)
(lib.dev.mkNixago lib.cfg.lefthook)
(lib.dev.mkNixago lib.cfg.adrgen)
(lib.dev.mkNixago cell.configs.cog)
];

I would really appreciate some compat with the original approach, with nil_ls being developed, the original approach should make it easier to implement attr properties hints from the LSP, which can be useful to onboard new users.

(It also encourages me to upgrade to the latest std so that it won't break my current config)

@Pegasust
Copy link
Contributor

My apologies for the oversight, seems like mkNixago now allows for adding modules incrementally. I could just do treefmt_scheme = dev.mkNixago cfg.treefmt then enjoy treefmt_config = treefmt_scheme raw_config.

Now, my nixago field looks just like what you showed earlier

  default = inputs.std.lib.dev.mkShell {
    name = "default shell";

    nixago = [
      (cell.config.treefmt)
      (cell.config.editorconfig)
      (cell.config.mdbook)
    ];

    commands = [
      cmd_mdbook #(from outer scope)
    ];
    imports = [inputs.std.std.devshellProfiles.default];
  };

Thanks for the good work!

@blaggacao
Copy link
Collaborator Author

blaggacao commented Jun 23, 2023

Thanks for cross posting. I indeed wanted to mention this Issue in the commit, but forgot to do so 😄

The full extend of this issue implies re-ification of those modules at the last possible moment, instead of injecting hard breaks to composability via mk{Nixago,Shell} et al.

@purepani
Copy link

purepani commented Sep 26, 2023

Is there any reason that block types can't also support defining a reification(or multiple) that can be accessed through nix?
For example, in this case, that would allow for just defining the config, and all of the evaluation can be ushered into the block type.
Right now block types(from what I understand) support defining actions, but that only ends up interacting with the cli.

cellBlocks = with std.blockTypes; [
        # Development Environments
        (nixago "configs")
        (devshells "shells")
      ];
    };

and the config above would be enough.

Or another way to put this would be: what if targets weren't just determined by the cell block, but also by the cell block type, so that the cell block type determines what the targets actually are, while the cell block simply defines that configuration?

Rereading the issue, I see that this may be exactly what the issue is proposing, but then I'm unsure why harvest wouldn't be able to pick that up if you were to, say, specify an action for it to perform.

@blaggacao
Copy link
Collaborator Author

blaggacao commented Sep 28, 2023

Implementing this in the "postprocessing" pipeline that ends up in __std is not much more than an API change.

However, in the std tree (<system>.*) we still want the still composable (!) raw data.

It's this need for an api change that had so far held me back to just do the change 😄

@purepani
Copy link

purepani commented Sep 28, 2023

Implementing this in the "postprocessing" pipeline that ends up in __std is not much more than an API change.

However, in the std tree (<system>.*) we still want the still composable (!) raw data.

I think the way I was imagining it was slightly different. There are currently 2 different namespaces(starting from the top level flake) to consider:
outputs.__std and <system>.*. I don't completely understand why __std would need an api change, since you already have an actions attribute for it, and the reification actions could just get thrown in there. The only change that I thought might be needed is changing the std tree to support something like:

${system}.${cell}.${block}.${target}.${action}

and leaving the action out would just result in the target in the same way it does now. That way, any existing configurations wouldn't be broken(until you update cell block types).

Then, when defining a block type, there would be 2 types of actions: those run within the CLI and those run within nix itself. The former would just be the exact same as currently, but the latter would provide reification. The first type would also be able to access the results of the second type(in this model).

This way, now the __std namespace should look exactly the same, as well as std tree being consistent with the one before. The only difference is now just the reification actions are held in the block type instead of in the blocks themselves.

I'm not sure if this is what you're imagining the change to look like, but I think it solves all the issues while keeping the api consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 Status: Review Needed This issue is pending a review 💪 Effort: 3 This issue is of medium complexity or only partly well understood
Projects
None yet
Development

No branches or pull requests

3 participants