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

broot: use upstream defaults, allow all config options #2644

Merged
merged 6 commits into from
Sep 26, 2022

Conversation

dermetfan
Copy link
Collaborator

@dermetfan dermetfan commented Jan 16, 2022

Description

The module did not allow to set all possible config options. It is now a freeform module in addition to the typed options that were already declared.

Default verbs were hard-coded and out of date (closes #2395) and therefore a maintenance burden. The default config from upstream is now used. Unfortunately this requires IFD.

I also changed the shell functions to be generated using IFD while I was at it so they won't have to be manually updated in the future. I do realize this may not be desired because they probably do not change often. I am fully happy removing that commit if anyone wishes.

Checklist

  • Change is backwards compatible.
    Partly: The Nix options interface is, but the resulting config file will be based on upstream's defaults now.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module It does not, but the original author is not using broot anymore.

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@dermetfan dermetfan requested a review from rycee as a code owner January 16, 2022 21:35
@dermetfan dermetfan changed the title Broot config broot: use upstream defaults, allow all config options Jan 16, 2022
Copy link
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

docs/release-notes/rl-2105.adoc Show resolved Hide resolved
modules/programs/broot.nix Outdated Show resolved Hide resolved
modules/programs/broot.nix Outdated Show resolved Hide resolved
modules/programs/broot.nix Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Apr 27, 2022

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@dermetfan
Copy link
Collaborator Author

@rycee Sorry to ping you again. We need a decision to continue this: Is IFD allowed in home-manager? If so, which restrictions apply?

@rycee
Copy link
Member

rycee commented Jun 26, 2022

Oh, about IFD. I have nothing against it and use it all the time. The main concern would be performance and clarity. So in HM I would avoid using it in excess and with slow to build derivations. I would also avoid using them when a non-IFD solution exists of comparable code clarity.

So in this particular case I think we could try avoiding it by letting Bash source the generated file since it would be equally clear (arguably even more so).

@dermetfan
Copy link
Collaborator Author

Thanks for your guidance! This could also help future decisions.

With this said, I think this PR is ready.

modules/programs/broot.nix Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Sep 25, 2022

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Sep 25, 2022
@dermetfan
Copy link
Collaborator Author

I'm merging this on my own to avoid bothering others.

All expressed concerns have been addressed and a decision about IFD was made.
I've been using this since I opened the PR without issue.
There have been no further comments until the bot marked the PR stale.

@stale stale bot removed the status: stale label Sep 26, 2022
@dermetfan dermetfan merged commit 65b65ce into nix-community:master Sep 26, 2022
@dermetfan dermetfan deleted the broot-config branch September 26, 2022 18:36
@rycee
Copy link
Member

rycee commented Sep 26, 2022

Yep, good work!

Btw, does the test build for you? I seem to get an error

FileNotFoundError: [Errno 2] No such file or directory: '/nix/store/dikbik0phcq4lf7al6s1g6imcx2grfqs-broot-1.15.0.tar.gz/resources/default-conf.hjson'

@K900
Copy link

K900 commented Sep 27, 2022

The latest broot update moved the configuration file to $src/resources/default-conf/conf.hjson

@dermetfan
Copy link
Collaborator Author

Sorry, I should have run the tests again given the age of this PR. Thankfully #3273 fixes this.

austinharris pushed a commit to austinharris/home-manager that referenced this pull request Dec 23, 2022
…#2644)

* broot: use freeformType for config

* broot: use defaults from upstream

closes nix-community#2395

* broot: generate shell function

* broot: add @dermetfan to CODEOWNERS

* broot: rename `config` option to `settings`

* broot: make example more idiomatic

Co-authored-by: Nicolas Berbiche <nic.berbiche@gmail.com>

Co-authored-by: Nicolas Berbiche <nic.berbiche@gmail.com>
spacekookie pushed a commit to spacekookie/home-manager that referenced this pull request Feb 10, 2023
…#2644)

* broot: use freeformType for config

* broot: use defaults from upstream

closes nix-community#2395

* broot: generate shell function

* broot: add @dermetfan to CODEOWNERS

* broot: rename `config` option to `settings`

* broot: make example more idiomatic

Co-authored-by: Nicolas Berbiche <nic.berbiche@gmail.com>

Co-authored-by: Nicolas Berbiche <nic.berbiche@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

broot: Make the defaults the defaults
5 participants