arrterian / nix-env-selector

Allows switch environment for Visual Studio Code using Nix Package Manager.
MIT License
222 stars 28 forks source link

allow specifying shell atts via config #13

Closed dsturnbull closed 4 years ago

dsturnbull commented 4 years ago
Status Type Config Change
Ready Feature Yes

Problem

Lots of nix shell configs require specifying an attribute.

Solution

Added a new config option where you can set the attr used.

New config parameters:

arrterian commented 4 years ago

@dsturnbull It's a really useful feature. Thanks a lot for your contribution. I left a review for your PR. Good job 🙂

dsturnbull commented 4 years ago

Thanks @arrterian. All these changes are fine, and in fact I started off with non-none Option type. The problem I can't figure out is how to use it in selectEnvCommandHander.

Please have a look at https://github.com/dsturnbull/nix-env-selector/pull/1 and see if you can see what the issue is :)

arrterian commented 4 years ago

@dsturnbull ohh miss one thing in the first review.

Action applyEnvByNixConfPath doesn't require any change. Just return original implementation and all should work fine.

Let's see on the current implementation

export const applyEnvByNixConfPath = (
  makeCmd: (path: string) => Option<string>
) => (nixConfigPath: string) =>
    of<Error, Option<string>>(makeCmd(nixConfigPath))
      .map(
        mapNullable(cmd =>
          node<Error, string>(done => exec(cmd, done))
            .map(parseEnv)
            .map(applyEnv)
        )
      )
      .chain(fold(() => of(false), id => id));

We have the only path arg applied to makeCmd and if we want to add the attribute to the CMD we need change only callback for creating the command, but not action executor. You changed both in your PR.

Now, lets see for getShellCmd type:

(internalCommand: "env", attr: Option<string>) => (path: string) => Option<string>

To satisfy our makeCmd type we just should apply this (internalCommand: "env", attr: Option<string>). You already have that in this line. https://github.com/dsturnbull/nix-env-selector/pull/1/commits/aeca6546f1e083a77d7e1b856654177ded3e3f7b#diff-45327f86d4438556066de133327f4ca2R53

So, to get the feature fully working just return current implementation for applyEnvByNixConfPath action.

dsturnbull commented 4 years ago

@arrterian thanks, please have a look now. :)

arrterian commented 4 years ago

@dsturnbull Fine! Just remove unused imports.

And one more thing before merging to the main codebase. Can you do rebase and squash the feature to a single commit? This need to avoid fixes message in history.

https://www.internalpointers.com/post/squash-commits-into-one-git

arrterian commented 4 years ago

@dsturnbull released

dsturnbull commented 4 years ago

Cheers @arrterian!