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

Add --print-build-logs to flake transient #150

Merged
merged 1 commit into from
Feb 28, 2022
Merged

Add --print-build-logs to flake transient #150

merged 1 commit into from
Feb 28, 2022

Conversation

chasecaleb
Copy link
Contributor

I find this option essential for nix build, so I added it to the flake transient.

Copy link
Contributor

@akirak akirak left a comment

Choose a reason for hiding this comment

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

Thank you for sending this PR. I think it is a good idea to add --print-build-logs. Maybe it should be the default? It would be better for us to allow customization of the default options, but this PR looks good to me.

Regarding the keybinding, I would suggest -L instead. When I first implemented this library, I chose to use a two-letter sequence for options that consist of multiple words (like -nu for --no-update-lock-file). Even with this rule, there will be conflicts if we add support for more options. For example, --sandbox, --store, and --substituters can conflict for s. Maybe we need better rules for abbreviating options, but we don't need to discuss short options (like -L) that are available in the CLI.

nix-flake.el Outdated Show resolved Hide resolved
Also make it enabled by default.
@chasecaleb
Copy link
Contributor Author

I agree with both your points. I changed it to -L and also made it enabled by default.

It would be better for us to allow customization of the default options

Users can already set defaults using transient-save, which is bound to C-x C-s within transients. I didn't know about it for a long time until I went hunting though, and I suspect a lot of other transient users don't know about it as well, so maybe you could mention it in your readme or something along those lines to raise awareness.


Thanks for the quick feedback, by the way!

@akirak
Copy link
Contributor

akirak commented Feb 27, 2022

@chasecaleb

Users can already set defaults using transient-save, which is bound to C-x C-s within transients.

Thank you for the information. I knew transient has many features, but I didn't dig into it.

I don't have an admin permission on this repository, nor can @matthewbauer give the permission to me, so please wait for a further response.

@matthewbauer matthewbauer merged commit 20ee8d8 into NixOS:master Feb 28, 2022
@chasecaleb chasecaleb deleted the add-print-build-logs-arg branch February 28, 2022 18:45
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.

None yet

3 participants