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.

Sign up for GitHub

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

Jump to bottom

zsh: refactor zsh configuration for better order control over .zshrc #6479

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

zsh: refactor zsh configuration for better order control over .zshrc #6479

wants to merge 4 commits into from
+178 −118

Conversation

Emin017
Copy link

@Emin017 Emin017 commented Feb 17, 2025 •
edited
Loading

Description

  • Refactor zsh.nix to use mkMerge and mkOrder for better control over .zshrc content ordering
  • Users can add content anywhere by using lib.mkOrder, lib.mkBefore and lib.mkAfter custom configurations
  • Added new test files (zshrc-contents-insert.nix, zshrc-contents-insert-before.nix, zshrc-contents-insert-after.nix and zshrc-contents-init-zprof.nix) to verify the behavior of initExtra with mkBefore, mkAfter, and default insertion.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all
    or nix build --reference-lock-file flake.lock ./tests#test-all using Flakes.

  • 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

    • Added myself as module maintainer. See example.

Maintainer CC

@github-actions github-actions bot added the shell label Feb 17, 2025
@Emin017 Emin017 force-pushed the zsh-shellInitLast branch 2 times, most recently from 394b2d9 to 315ce87 Compare February 17, 2025 08:18
@Emin017 Emin017 changed the title zsh: implement shellInitLast zsh: implement initExtraLast Feb 17, 2025
teto
teto requested changes Feb 17, 2025
View reviewed changes
Copy link
Collaborator

@teto teto left a comment

Choose a reason for hiding this comment

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

this is getting out of hand. We would have
initExtraBeforeCompInit
initExtraLast
initExtra
. I suggest you split all the additions to zshrc with various mkOrder so one can insert his code wherever he wants

@Emin017 Emin017 marked this pull request as draft February 19, 2025 13:39
@Emin017 Emin017 changed the title zsh: implement initExtraLast zsh: add initContents option for custom .zshrc content insertion Feb 20, 2025
@Emin017 Emin017 marked this pull request as ready for review February 20, 2025 14:10
@Emin017 Emin017 marked this pull request as draft February 20, 2025 14:37
@Emin017 Emin017 marked this pull request as ready for review February 21, 2025 00:14
@Emin017 Emin017 requested a review from teto February 21, 2025 00:14
teto
teto reviewed Mar 4, 2025
View reviewed changes
@teto
Copy link
Collaborator

teto commented Mar 4, 2025

thanks for taking on this huge work. I am quite looking forward to it.

@teto teto mentioned this pull request Mar 4, 2025
Open
@Emin017 Emin017 force-pushed the zsh-shellInitLast branch from 776daea to f1fdc33 Compare March 5, 2025 14:58
@Emin017 Emin017 changed the title zsh: add initContents option for custom .zshrc content insertion zsh: refactor zsh configuration for better order control over .zshrc Mar 5, 2025
teto
teto reviewed Mar 5, 2025
View reviewed changes
@teto
Copy link
Collaborator

teto commented Mar 5, 2025

on a quick glance it looks fine by me. I have follow up ideas but merging this first is better, it's the hardest part. diffing my ~/.zshrc with and without this just shows a few different newlines which is fine. I would like to let this cook a few days to give the opportunity for users with different zsh setups test this since it's a "dangerous" change :)

teto
teto reviewed Mar 8, 2025
View reviewed changes
@teto
Copy link
Collaborator

teto commented Mar 8, 2025

sry I intended to merge than noticed about the lib.mkForce issue. You will have to rebase sorry but I will try to merge it fast after that. Could you maybe mention in the description of initExtra how to insert zsh code with lib.mkOrder ?

@Emin017
Copy link
Author

Emin017 commented Mar 9, 2025

home.file."${relToDotDir ".zshrc"}".text = mkBefore would still work but yeah that's less visible from the doc. Putting all the generated code in initExtra feels odd since it's not extra anymore ? In a previous version of the PR @Emin017 had added a new content option, maybe that's we need after all to keep backwards compatibility.

Should I need to switch back to the previous version?

sry I intended to merge than noticed about the lib.mkForce issue. You will have to rebase sorry but I will try to merge it fast after that. Could you maybe mention in the description of initExtra how to insert zsh code with lib.mkOrder ?

Sure, I will add examples and descriptions about this.

@khaneliman khaneliman mentioned this pull request Mar 10, 2025
Merged
6 tasks
@khaneliman
Copy link
Collaborator

Will be nice to help with an issue #6572 Currently, we are inserting into initExtra which isn't far enough down in the generated file.

@teto
Copy link
Collaborator

teto commented Mar 10, 2025

Should I need to switch back to the previous version?

@Emin017 I apologize for my bad advice, yes please restore the content option you had added.

@teto
Copy link
Collaborator

teto commented Mar 10, 2025

I am not a fan of modules/misc/news.nix in general but this change is worth adding to it as well.

@Emin017 Emin017 marked this pull request as draft March 10, 2025 14:30
@Emin017 Emin017 force-pushed the zsh-shellInitLast branch from 2a9165b to b5f8c61 Compare March 11, 2025 02:10
khaneliman
khaneliman reviewed Mar 11, 2025
View reviewed changes
tests/modules/programs/zsh/default.nix Outdated
Comment on lines 13 to 15
zshrc-contents-insert = ./zshrc-contents-insert.nix;
zshrc-contents-insert-before = ./zshrc-contents-insert-before.nix;
zshrc-contents-insert-after = ./zshrc-contents-insert-after.nix;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's worth the extra CI time for these tests, in their current state. It would be better to have a single test that adds to the initContent with different priorities and verifying they are generated in the expected order.

Copy link
Author

Choose a reason for hiding this comment

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

I merged them into a single priorities test in the new commit.

@Emin017
zsh: implement initExtraLast
26cdc14 
Add a new option called `initExtraLast` to the zsh module. This option
allows users to specify shell script code that will be executed as the
last step during interactive zsh shell initialization.

Signed-off-by: Qiming Chu <[email protected]>
@Emin017 Emin017 force-pushed the zsh-shellInitLast branch from b5f8c61 to 9599c74 Compare March 11, 2025 06:04
Emin017 added 3 commits March 11, 2025 14:36
@Emin017
zsh: add initContent option for custom .zshrc content insertion
1fd44cc 
- Users can add content anywhere by using `lib.mkOrder`, `lib.mkBefore`
and `lib.mkAfter` custom configurations.
- Add test cases to verify the insertion of content before and after
existing configurations in `.zshrc`.

Signed-off-by: Qiming Chu <[email protected]>
@Emin017
zsh: add example, description and news for programs.zsh.initContent
e27fc0d 
Signed-off-by: Qiming Chu <[email protected]>
@Emin017
zsh: consolidate zshrc content tests into a single priorities test
f8c1c62 
Signed-off-by: Qiming Chu <[email protected]>
@Emin017 Emin017 force-pushed the zsh-shellInitLast branch from 9599c74 to f8c1c62 Compare March 11, 2025 06:38
@Emin017 Emin017 marked this pull request as ready for review March 11, 2025 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teto teto teto approved these changes

@khaneliman khaneliman khaneliman left review comments

Assignees
No one assigned
Labels
merge conflict shell
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Emin017 @teto @khaneliman