discord: combine nixcord, vencord, vesktop #854

Flameopathic · 2025-02-10T20:51:23Z

combines the nixcord, vencord, and vesktop hm.nix modules into one.

handles the config separately as the nixcord module requires odd conditions for evaluation.

unsure how to handle the testbeds.

trueNAHO · 2025-02-10T21:10:42Z

unsure how to handle the testbeds.

@danth, /modules/firefox/ suffers from the same problem of having one testbed per module, although multiple testbeds would be beneficial.

What do you think of optional /modules/<MODULE>/testbeds/<TESTBED>.nix testbeds, while asserting that /modules/<MODULE>/testbed.nix should not exist while any /modules/<MODULE>/testbeds/<TESTBED>.nix exists. However, this prevents the use of a highly convenient local /modules/<MODULE>/testbeds/common.nix abstraction. Either, we handle certain file names differently (probably a bad idea) or such files are expected to go to /modules/<MODULE>/common.nix. Since we are looking for specific files and directories inside /modules/<MODULE>/, adding arbitrary named files in this scope does not cause problems.

Note that implementing this functionality should probably not fall in the scope of this PR. It should be fine to keep only one testbed in this PR and add the others back once this functionality is implemented.

danth · 2025-02-11T12:37:44Z

@trueNAHO That sounds reasonable. Ignoring specific file names would be confusing and limiting, so I agree, it's better to just import from the parent directory when some shared code is needed.

modules/discord/hm.nix

trueNAHO · 2025-02-11T22:44:15Z

Btw, I am currently working on #854 (comment) and will finish it up tomorrow.

trueNAHO · 2025-02-12T00:53:36Z

Btw, I am currently working on #854 (comment) and will finish it up tomorrow.

If we wait for #858 to get merged, we can keep all testbeds in this PR.

Flameopathic · 2025-02-12T13:15:33Z

If we wait for #858 to get merged, we can keep all testbeds in this PR.

i believe i've put the testbeds in the right location for when it is.

trueNAHO

If we wait for #858 to get merged, we can keep all testbeds in this PR.

i believe i've put the testbeds in the right location for when it is.

I just realized that I intended the testbed directory to be called testbeds instead of testbed: f79d6cf93e0b..482525df19c8.

Otherwise, this PR LGTM once #858 is merged.

Flameopathic · 2025-02-12T16:23:16Z

I just realized that I intended the testbed directory to be called testbeds instead of testbed: f79d6cf93e0b..482525df19c8.

resolved!

trueNAHO

LGTM.

Waiting on #858 to merge this PR.

danth · 2025-02-12T23:57:19Z

Note that with the current state of #858, the testbeds will end up being named testbed-discord-{vencord,vesktop}, which doesn't correspond with the stylix.targets.{vencord,vesktop} options. Personally I would expect the testbeds to be named testbed-{vencord,vesktop} as they are separate targets.

Not sure if this is really an issue or I'm just being picky.

Flameopathic · 2025-02-13T01:06:33Z

Personally I would expect the testbeds to be named testbed-{vencord,vesktop} as they are separate targets.

I think that this would be better, if possible

trueNAHO · 2025-02-15T18:52:48Z

testbeds will end up being named testbed-discord-{vencord,vesktop}, which doesn't correspond with the stylix.targets.{vencord,vesktop} options. Personally I would expect the testbeds to be named testbed-{vencord,vesktop} as they are separate targets.

[...]

Not sure if this is really an issue or I'm just being picky.

Actually, indicating in the testbed name that the vencord and vesktop testbed variants are conceptually linked via the discord node seems like a bonus for me.

danth · 2025-02-16T21:11:05Z

Depends on whether we want to match the target name (improves user experience) or the module name (improves developer experience).

trueNAHO

Waiting on #858 to merge this PR.

This has been merged in commit 211a844.

danth

Looks good to me.

#858 adds to the documentation that testbeds should be named after the module, not the target: see this comment.

#873 also clarifies this on the user side:

stylix/docs/src/configuration.md

Lines 207 to 211 in 73bcf4a

    
           You can discover the available targets and their options by browsing through 
        
           the module reference at the end of this book. Most targets will be found under 
        
           a module of the same name, but occasionally a module will serve multiple similar 
        
           targets. For example, the [Firefox module](options/modules/firefox.md) also 
        
           provides options for other browsers which are based on Firefox.

Hence the current naming of the testbeds should be fine.

danth · 2025-02-17T20:49:23Z

@trueNAHO did you mean to request changes? I don't see anything else which needs to be done.

trueNAHO

@trueNAHO did you mean to request changes? I don't see anything else which needs to be done.

My bad. This PR LGTM.

Link: danth#854 Reviewed-by: Daniel Thwaites <[email protected]> Reviewed-by: NAHO <[email protected]>

Link: #885 Fixes: 7feb1c2 ("discord: combine nixcord, vencord, and vesktop modules (#854)") Reviewed-by: NAHO <[email protected]>

Link: danth#885 Fixes: 7feb1c2 ("discord: combine nixcord, vencord, and vesktop modules (danth#854)") Reviewed-by: NAHO <[email protected]>

Flameopathic force-pushed the discord-module branch from 3af1a63 to 92777e2 Compare February 10, 2025 20:51

Flameopathic marked this pull request as draft February 10, 2025 20:56

Flameopathic force-pushed the discord-module branch 2 times, most recently from c8ce86d to f0ad74f Compare February 10, 2025 20:59

Flameopathic force-pushed the discord-module branch 2 times, most recently from f950658 to e6d3fd8 Compare February 11, 2025 02:28

Flameopathic marked this pull request as ready for review February 11, 2025 02:33

danth reviewed Feb 11, 2025

View reviewed changes

modules/discord/hm.nix Outdated Show resolved Hide resolved

Flameopathic force-pushed the discord-module branch from e6d3fd8 to f7df7f3 Compare February 11, 2025 14:23

trueNAHO requested changes Feb 11, 2025

View reviewed changes

modules/discord/hm.nix Outdated Show resolved Hide resolved

modules/discord/hm.nix Show resolved Hide resolved

Flameopathic force-pushed the discord-module branch 2 times, most recently from 3391373 to 096b3f0 Compare February 12, 2025 02:30

discord: combine nixcord, vencord, vesktop

dfe24d5

Flameopathic force-pushed the discord-module branch from 096b3f0 to dfe24d5 Compare February 12, 2025 13:01

trueNAHO requested changes Feb 12, 2025

View reviewed changes

change directory to testbeds

5e5e327

trueNAHO approved these changes Feb 12, 2025

View reviewed changes

danth mentioned this pull request Feb 16, 2025

stylix: support multiple testbeds per module #858

Merged

trueNAHO requested changes Feb 17, 2025

View reviewed changes

danth approved these changes Feb 17, 2025

View reviewed changes

trueNAHO approved these changes Feb 17, 2025

View reviewed changes

trueNAHO merged commit 7feb1c2 into danth:master Feb 17, 2025
49 checks passed

pinarruiz pushed a commit to pinarruiz/stylix that referenced this pull request Feb 19, 2025

discord: combine nixcord, vencord, and vesktop modules (danth#854)

0b6aa6d

Link: danth#854 Reviewed-by: Daniel Thwaites <[email protected]> Reviewed-by: NAHO <[email protected]>

pinarruiz pushed a commit to pinarruiz/stylix that referenced this pull request Feb 19, 2025

discord: combine nixcord, vencord, and vesktop modules (danth#854)

8bceb5c

Link: danth#854 Reviewed-by: Daniel Thwaites <[email protected]> Reviewed-by: NAHO <[email protected]>

danth mentioned this pull request Feb 20, 2025

discord: remove duplicate testbeds #885

Merged

trueNAHO pushed a commit that referenced this pull request Feb 21, 2025

{vencord,vesktop}: remove duplicate testbeds (#885)

8b6edfd

Link: #885 Fixes: 7feb1c2 ("discord: combine nixcord, vencord, and vesktop modules (#854)") Reviewed-by: NAHO <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discord: combine nixcord, vencord, vesktop #854

discord: combine nixcord, vencord, vesktop #854

Flameopathic commented Feb 10, 2025 •

edited

Loading

trueNAHO commented Feb 10, 2025 •

edited

Loading

danth commented Feb 11, 2025

trueNAHO commented Feb 11, 2025

trueNAHO commented Feb 12, 2025

Flameopathic commented Feb 12, 2025

trueNAHO left a comment

Flameopathic commented Feb 12, 2025

trueNAHO left a comment

danth commented Feb 12, 2025

Flameopathic commented Feb 13, 2025

trueNAHO commented Feb 15, 2025

danth commented Feb 16, 2025

trueNAHO left a comment

danth left a comment

danth commented Feb 17, 2025

trueNAHO left a comment

	You can discover the available targets and their options by browsing through
	the module reference at the end of this book. Most targets will be found under
	a module of the same name, but occasionally a module will serve multiple similar
	targets. For example, the [Firefox module](options/modules/firefox.md) also
	provides options for other browsers which are based on Firefox.

discord: combine nixcord, vencord, vesktop #854

discord: combine nixcord, vencord, vesktop #854

Conversation

Flameopathic commented Feb 10, 2025 • edited Loading

trueNAHO commented Feb 10, 2025 • edited Loading

danth commented Feb 11, 2025

trueNAHO commented Feb 11, 2025

trueNAHO commented Feb 12, 2025

Flameopathic commented Feb 12, 2025

trueNAHO left a comment

Choose a reason for hiding this comment

Flameopathic commented Feb 12, 2025

trueNAHO left a comment

Choose a reason for hiding this comment

danth commented Feb 12, 2025

Flameopathic commented Feb 13, 2025

trueNAHO commented Feb 15, 2025

danth commented Feb 16, 2025

trueNAHO left a comment

Choose a reason for hiding this comment

danth left a comment

Choose a reason for hiding this comment

danth commented Feb 17, 2025

trueNAHO left a comment

Choose a reason for hiding this comment

Flameopathic commented Feb 10, 2025 •

edited

Loading

trueNAHO commented Feb 10, 2025 •

edited

Loading