-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: barebones dependency conversion #61
base: main
Are you sure you want to change the base?
Conversation
c2d625a
to
7c818ed
Compare
describe('with no gitmodules file', function () { | ||
before(() => { | ||
mock({ | ||
'.gitmodules': '', |
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.
Do we need the mock here? I suspect it will make it look like the file exists and is empty, and ideally we should test the case of the file not being present at all, in particular to ensure that the read call doesn't revert.
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.
good find 🤠 fixed it in cd631e6
5f90b8c
to
6993ff6
Compare
cd631e6
to
c4b2b29
Compare
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.
LGTM!
expect(parsedDependencies).to.deep.eq({}); | ||
}); | ||
}); | ||
describe('with a file with several dependencies', function () { |
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.
Maybe worth also having a case where the gitmodules exist but is not in the format we expect it
merge after #58
closes #60
adding support for git dependencies means we'll probably also support projects where the dependencies are not explicitly stated in
remappings.txt
, the build instead relying on foundry's implicit remappings. To fully handle those cases, this project should invokeforge remappings
instead of readingremappings.txt
TODO
transformRemappings
.gitmodules
parsingI tried to use the ini npm package since
.gitmodules
seem to be .ini files, but didn't behave as expected.It didn't recognize the various
path
andurl
fields as part of the relevantsubmodule
subsections, instead assignig them as items ofsubmodule
, so after parsing an entire file only the last two remained:would yield
The current implementation works for machine-generated
.gitmodules
, but will break if a subseciton is populated in two passes like so:or if path and url are out of order (which is perfectly valid for git)
and probably other cases that are syntactically valid ini, and sematically valid submodule definitions, but that dont match my arbitrary regex
I deem this good enough for now, specially considering we are doing the same problematic regex-parsing for solidity files