Skip to content

feat(modules.constructFiles): auto-sanitize .key value to always be valid#399

Merged
BirdeeHub merged 1 commit into
mainfrom
constructFiles
Mar 30, 2026
Merged

feat(modules.constructFiles): auto-sanitize .key value to always be valid#399
BirdeeHub merged 1 commit into
mainfrom
constructFiles

Conversation

@BirdeeHub
Copy link
Copy Markdown
Owner

@BirdeeHub BirdeeHub commented Mar 30, 2026

Is this a good idea? Thumbs up or thumbs down to vote I suppose?

@zenoli
Copy link
Copy Markdown
Contributor

zenoli commented Mar 30, 2026

Will we ever have to look at the sanitized keys?
If no, why not generate a hash or youse some encoding function that ensures we will never have collisions (base64 wouldnt work but there might be something that encodes to just [A-Za-z0-9_].

If it's important that the sanitize function results in something human-readable and there is the possibility for collisions, I would at the very least want a safety hatch where we could override things in case of collisions.

@BirdeeHub
Copy link
Copy Markdown
Owner Author

BirdeeHub commented Mar 30, 2026

They are in config.drv

But, you don't actually need to use those values, you would modify them via the constructFiles options for them anyway

You just need to not overwrite them on accident.

where we could override things in case of collisions

When a collision occurs, it adds _0, _1, etc...

And if you set the value to a valid shell variable name, it will not modify it, unless it collides with something

@zenoli
Copy link
Copy Markdown
Contributor

zenoli commented Mar 30, 2026

When a collision occurs, it adds _0, _1, etc...

And if you set the value to a valid shell variable name, it will not modify it, unless it collides with something

If no collisions can occur, then thumbs up by me :-)

Comment thread lib/lib.nix Outdated

Examples:
```
sanitizeEnvVarName "FOO-BAR" => "FOOBAR"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can keep "_" as the only separator and make it become FOO_BAR (same for FOO.BAR, FOO+BAR etc)
This would certainly avoid a whole class of collisions where your workaround with _0, _1,... would be needed otherwise.

Copy link
Copy Markdown
Owner Author

@BirdeeHub BirdeeHub Mar 30, 2026

Choose a reason for hiding this comment

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

This would certainly avoid a whole class of collisions where your workaround with _0, _1,... would be needed otherwise.

It would not, although there would be fewer instances

I can do this though, just substitute all invalid characters with _, that is fine with me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm nitpicking.
I was just thinking this might reduce clashes like dotdir vs dot-dir but in the end its up to you.

In the end it doesn't really matter as it is hidden away from the user anyway.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

It actually simplified the sanitizeEnvVarName function to do it that way

@zenoli
Copy link
Copy Markdown
Contributor

zenoli commented Mar 30, 2026

@BirdeeHub can you ping me at #378 once this is merged s.t. I can clean things up using this?

@BirdeeHub BirdeeHub force-pushed the constructFiles branch 2 times, most recently from b6c670d to 88545f3 Compare March 30, 2026 13:01
@BirdeeHub BirdeeHub marked this pull request as draft March 30, 2026 13:25
@BirdeeHub BirdeeHub marked this pull request as ready for review March 30, 2026 13:25
@BirdeeHub BirdeeHub force-pushed the constructFiles branch 3 times, most recently from e4244b1 to cbc8d2c Compare March 30, 2026 13:50
…alid

should make it easier to map multiple things to generated files
@BirdeeHub BirdeeHub merged commit bab35ff into main Mar 30, 2026
2 checks passed
@BirdeeHub
Copy link
Copy Markdown
Owner Author

Ill clean up the existing modules I maintain where I needed to work around this eventually, but they are fine for now. But going forward this should be nicer. @zenoli Im merging this.

@BirdeeHub BirdeeHub deleted the constructFiles branch March 30, 2026 13:59
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.

2 participants