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

sanitizeDerivationName seems unnecessary #260

Closed
wmertens opened this issue Aug 20, 2022 · 6 comments
Closed

sanitizeDerivationName seems unnecessary #260

wmertens opened this issue Aug 20, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@wmertens
Copy link
Contributor

there's

{
  sanitizeDerivationName = name:
    lib.replaceStrings ["@" "/"] ["__at__" "__slash__"] name;
}

which results in long and unreadable names for modules like __at__babel__slash__runtime, and there's no technical need for it, since Nix will sanitize names like @babel/runtime to -babel-runtime.

How about not sanitizing derivation names?

@tinybeachthor
Copy link
Contributor

I think this makes sense.
I prefer the shorter name, and there is no real information lost between __at__babel__slash__runtime and -babel-runtime.

Could this change break anything though?

@wmertens
Copy link
Contributor Author

wmertens commented Sep 2, 2022

No breaking change, it only means rebuilding packages

@DavHau
Copy link
Member

DavHau commented Sep 5, 2022

I'm not opposed to removing the custom sanitizing. I just thought, that with -babel-runtime it might not be clear on first glance if this is a namespaced package or a package called babel-runtime, but actually the - indicates a namespace.
But if we had @babel/runtime-foo it would be rendered to -babel-runtime-foo, making it unclear if runtime is part of the namespace or package name.

@wmertens
Copy link
Contributor Author

wmertens commented Sep 5, 2022

In any case, RFC #17 proposes doing away with names in store paths, and the actual name of the package is evident in the lib/node_modules directory.

So I don't think there's much opportunity for confusion.

@DavHau DavHau added the enhancement New feature or request label Sep 8, 2022
@DavHau
Copy link
Member

DavHau commented Sep 8, 2022

OK, I'm fine with removing the use of sanitizDerivationName.

@tinybeachthor
Copy link
Contributor

Resolved in #315

@DavHau DavHau closed this as completed Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants