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

refactor name mangling #79

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

Conversation

AlexKnauth
Copy link
Contributor

This PR makes way for other namespaces such as the module and signature namespaces in the module language @iitalics and I are working on.

It splits up the logic into these files:

  • hackett/private/mangle/mangle-string.rkt handles mangling/unmangling of strings
  • hackett/private/mangle/mangle-identifier.rkt handles identifiers and introducing scopes
  • hackett/private/mangle/mangle-import-export.rkt deals with the import and export objects used by require and provide transformers
  • hackett/private/mangle/mangle-reqprov.rkt defines the transformers, parsing the keywords #:no-introduce, #:prefix, and #:only to call different functions from the other files

@lexi-lambda
Copy link
Owner

I’m sorry for not commenting on this sooner. I didn’t forget about it, but I should have gotten to it earlier. Hopefully two weeks late isn’t too late!

Anyway, to discuss the actual contents of this PR: bluntly, this seems like a lot of added complexity in the Hackett codebase for something that is used in precisely one place. That is to say, in the absence of multiple consumers within Hackett itself, it feels like strongly premature abstraction to me.

Now, I do understand that this is useful to you, since you’re adding more namespaces and actually do have more than one consumer, but I’m reluctant to take on the complexity overhead of abstracting this stuff out in the mainline repo until there is a need to. Does that make sense?

To put it another way: is there some reason these changes can’t stay on your branch for now? Or is there a reason you really need them merged into Hackett itself right now?

@AlexKnauth
Copy link
Contributor Author

What we currently have is a copy-pasted version of this code in hackett-module. Right now it "happens to work" because the mangling scheme "happens" to be compatible with hackett's mangling scheme. Having this be merged in to Hackett would change "happens to work because implementation details" into "works because interface".

But if "happens to work because implementation details" is good enough, this PR is not completely necessary.

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.

3 participants