Skip to content

Improve i3.nix to handle options as list like in sway, adjusted functions for less new-lines#2314

Merged
sumnerevans merged 2 commits intonix-community:masterfrom
SebTM:i2030_i3_refacotring_newlines
Mar 21, 2022
Merged

Improve i3.nix to handle options as list like in sway, adjusted functions for less new-lines#2314
sumnerevans merged 2 commits intonix-community:masterfrom
SebTM:i2030_i3_refacotring_newlines

Conversation

@SebTM
Copy link
Copy Markdown
Collaborator

@SebTM SebTM commented Sep 5, 2021

Description

Issue #2030 should describe it well :)
Didn't fixed the tests yet, just wanted to ask if there is feedback before about the way2go here? :)

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.
    Yes but formats some code parts really bad?

  • 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

    • Added myself as module maintainer. See example.

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

@SebTM SebTM requested a review from sumnerevans as a code owner September 5, 2021 11:13
Copy link
Copy Markdown
Contributor

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

Seems like the right direction. Please update the tests to make sure that you didn't introduce any regressions.

Comment thread modules/services/window-managers/i3-sway/lib/functions.nix Outdated
@SebTM SebTM requested a review from alexarice as a code owner September 17, 2021 14:38
@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Sep 17, 2021

Fixed the tests, format and adjusted the writeText in i3/sway to follow this approach. Would be nice if you could review it again :)

@SebTM SebTM requested a review from sumnerevans September 21, 2021 12:21
Copy link
Copy Markdown
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.

Looks good overall.
Would be cool if the indentation could also be preserved.

Comment thread modules/services/window-managers/i3-sway/i3.nix Outdated
Comment thread modules/services/window-managers/i3-sway/lib/functions.nix Outdated
Comment thread modules/services/window-managers/i3-sway/lib/functions.nix Outdated
Comment thread modules/services/window-managers/i3-sway/lib/functions.nix Outdated
Comment thread modules/services/window-managers/i3-sway/i3.nix
Comment thread modules/services/window-managers/i3-sway/i3.nix
@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Nov 23, 2021

Looks good overall. Would be cool if the indentation could also be preserved.

Tried my best in the new commits :)

@SebTM SebTM requested a review from rycee as a code owner November 23, 2021 21:38
@SebTM SebTM requested a review from berbiche November 23, 2021 21:40
@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Nov 24, 2021

Changed everything according feedback, one conversation left because of some things to check back :)
Kudo's to @Ma27 for supporting 🥇

Copy link
Copy Markdown
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.

Could you move the lists.nix changes to a different PR and undo the changes in this PR?

Comment thread modules/services/window-managers/i3-sway/i3.nix Outdated
Comment thread modules/services/window-managers/i3-sway/sway.nix Outdated
Comment thread modules/services/window-managers/i3-sway/lib/functions.nix Outdated
Comment thread modules/services/window-managers/i3-sway/lib/functions.nix Outdated
@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Dec 15, 2021

Could you move the lists.nix changes to a different PR and undo the changes in this PR?

Done, also did my best for the other points of your feedback :) This PR now depends on #2566 where the new lib-function is added (this is why the pipeline fails). Would appreciate your re-review 👍🏻

@SebTM SebTM requested a review from berbiche December 25, 2021 03:32
@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Dec 25, 2021

Rebased on current master, Cleaned commit :)

@sumnerevans
Copy link
Copy Markdown
Contributor

sumnerevans commented Jan 1, 2022

Looks like the build is failing for some reason. Can you look into that (oh, I see, it depends on the other PR)?

Copy link
Copy Markdown
Contributor

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

Looks good, overall.

Comment thread modules/services/window-managers/i3-sway/i3.nix Outdated
Comment thread modules/services/window-managers/i3-sway/i3.nix Outdated
Comment thread modules/services/window-managers/i3-sway/lib/functions.nix Outdated
@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Mar 2, 2022

Rebased to current master, applied changes from feedback, fixed tests and found lists.subtractLists which does exactly what I needed I guess :)

Tests seem to fail because of the issue in #2767 with PR #2770 pending ✌🏻

Comment thread modules/services/window-managers/i3-sway/sway.nix Outdated
@SebTM SebTM requested a review from sumnerevans March 17, 2022 04:21
Copy link
Copy Markdown
Contributor

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

Sorry it's taken so long to look over this. I think it looks fine. If you want to try and refactor to avoid the ++ [ "\n" ] then feel free to and I'll have another look.

@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Mar 21, 2022

Are the failing tests related? I would say no...

herbstluftwm-simple-config: FAILED
Expected /nix/store/hjxvnf979k037q74digq2k64w49z2n69-nmt-report-herbstluftwm-simple-config/normalized/autostart to be same as /nix/store/ci79wirs2xwxb23nfbd3cd14b0g4bzpv-autostart but were different:
--- actual
+++ expected
@@ -1,4 +1,4 @@
-#!/nix/store/00000000000000000000000000000000-bash-5.1-p16/bin/bash
+#!/nix/store/00000000000000000000000000000000-bash-5.1-p12/bin/bash
 shopt -s expand_aliases
 
 # shellcheck disable=SC2142
For further reference please introspect /nix/store/hjxvnf979k037q74digq2k64w49z2n69-nmt-report-herbstluftwm-simple-config

mpd-basic-configuration: FAILED
Expected /nix/store/pgj4vjhihn3vsx4n9865w4ik3vaqklkf-nmt-report-mpd-basic-configuration/normalized/mpd.service to be same as /nix/store/6ix31n8jxwzq5rzmyk0vdh8jkhs65y9j-basic-configuration.service but were different:
--- actual
+++ expected
@@ -4,7 +4,7 @@
 [Service]
 Environment=PATH=/home/hm-user/.nix-profile/bin
 ExecStart=@mpd@/bin/mpd --no-daemon /nix/store/00000000000000000000000000000000-mpd.conf
-ExecStartPre=/nix/store/00000000000000000000000000000000-bash-5.1-p16/bin/bash -c "/nix/store/00000000000000000000000000000000-coreutils-9.0/bin/mkdir -p '/home/hm-user/.local/share/mpd' '/home/hm-user/.local/share/mpd/playlists'"
+ExecStartPre=/nix/store/00000000000000000000000000000000-bash-5.1-p12/bin/bash -c "/nix/store/00000000000000000000000000000000-coreutils-9.0/bin/mkdir -p '/home/hm-user/.local/share/mpd' '/home/hm-user/.local/share/mpd/playlists'"
 Type=notify
 
 [Unit]
For further reference please introspect /nix/store/pgj4vjhihn3vsx4n9865w4ik3vaqklkf-nmt-report-mpd-basic-configuration

I've also added myself as maintainer and to CodeOwners for notifications on related things, if it's OK for your? (#2815 for adding me as maintainer)

@SebTM SebTM mentioned this pull request Mar 21, 2022
7 tasks
@SebTM
Copy link
Copy Markdown
Collaborator Author

SebTM commented Mar 21, 2022

Rebased (now including fixed tests) and added commit as mention in #2815 :)

@sumnerevans sumnerevans merged commit 80b4360 into nix-community:master Mar 21, 2022
@SebTM SebTM deleted the i2030_i3_refacotring_newlines branch March 22, 2022 01:06
jficz pushed a commit to jficz/home-manager that referenced this pull request Apr 7, 2022
…ted functions for less new-lines (nix-community#2314)

* i3/sway: Improve code to generate config to avoid new-line issues on code/config changes

* Maintainer: Add SebTM as maintainer
@teto teto mentioned this pull request Aug 22, 2022
7 tasks
teto pushed a commit to teto/home-manager that referenced this pull request Aug 22, 2022
…ted functions for less new-lines (nix-community#2314)

* i3/sway: Improve code to generate config to avoid new-line issues on code/config changes

* Maintainer: Add SebTM as maintainer
spacekookie pushed a commit to spacekookie/home-manager that referenced this pull request Feb 10, 2023
…ted functions for less new-lines (nix-community#2314)

* i3/sway: Improve code to generate config to avoid new-line issues on code/config changes

* Maintainer: Add SebTM as maintainer
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.

4 participants