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

Test.sol import statement changed #198

Closed
wants to merge 1 commit into from

Conversation

GokhanPolat
Copy link

Import file path changed to relative path. By the way, only this line was using this type of import statement (with remappings).

If we want to different remappings or even don't want to use remappings, some checker/analyzer tools can be confused that hard to understand of remappings. Nested libraries can be problematic.

My suggestion is using relative path for libraries but I'm open too other suggestions.

@mds1
Copy link
Collaborator

mds1 commented Oct 28, 2022

@mattsse do you have a preference on this? given your comment here: #195 (comment) (though I'm not sure if this has ever caused issues, since ds-test is pretty stable and everyone is using the same version anyway)

@GokhanPolat if we do want this change, let's change the target branch to the v0.3 branch of #184

@mattsse
Copy link
Member

mattsse commented Oct 28, 2022

I'm not aware of any issues re Test import.

But I guess using a relative import here ensures we always pull in the expected version.

there's a possibility that different ds-test remapping will override this, but as mentioned it's pretty stable.

But the motivation for this PR seems unrelated.
given that forge-std is bundled with ds-test I think we can safely make this change?

@mds1
Copy link
Collaborator

mds1 commented Oct 28, 2022

given that forge-std is bundled with ds-test I think we can safely make this change?

Yea, I think this should be safe, and I guess it can't hurt, so may as well do it? Though personally I don't like using relative paths hehe, but since forge-std is included by default, guess it's ok to err on the side of "ensure remappings always get correct ds-test version"

@GokhanPolat
Copy link
Author

@mds1

Though personally I don't like using relative paths

Absolutely agree with you. So how should we proceed?

@mds1
Copy link
Collaborator

mds1 commented Oct 28, 2022

I'm going to close this for now since I don't think it's needed—ds-test has been stable in this repo for quite some time, and we haven't heard reports of any issues resulting from it, let's leave this as-is

But if we ever update the ds-test submodule version, we can make this change at the same time to ensure the new one is used. Thanks @GokhanPolat!

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