-
Notifications
You must be signed in to change notification settings - Fork 60
Allow multiple library paths #2436
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable!
There is one breaking default in this PR that should be fixed.
Also: what happens when the same file name exists across many paths? I think we should generate an error instead of silently overriding an existing path file.
I've changed it so that it emits an error if it finds an import in multiple library paths. |
This pull request has not seen activity in 14 days and is being marked as stale. If you're continuing work on this, please reply and state how to get this PR in a mergeable state or what issues it is blocked on. If the PR is not ready for review, please mark it as a draft. The PR will be closed in 7 days if there is no further activity. |
@rachitnigam if the changes addressed your concerns then I'll go ahead and merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Left some random nits that you can address or not as you please
This makes it possible to specify multiple library paths, which is useful for frontends that wish to provide their own primitive libraries in addition to Calyx's standard library.