-
Notifications
You must be signed in to change notification settings - Fork 764
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(solc): add script/ to project paths #1359
Conversation
@@ -420,7 +437,8 @@ impl Default for ProjectPaths { | |||
Self { | |||
artifacts: "out".into(), | |||
sources: "src".into(), | |||
tests: "tests".into(), | |||
tests: "test".into(), |
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.
I changed here, because it seems like tests
was a mistake here. Should I revert it?
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.
that makes sense, is also the default of foundry::Config
@@ -538,7 +562,8 @@ impl ProjectPathsConfigBuilder { | |||
.artifacts | |||
.unwrap_or_else(|| ProjectPathsConfig::find_artifacts_dir(&root)), | |||
sources: self.sources.unwrap_or_else(|| ProjectPathsConfig::find_source_dir(&root)), | |||
tests: self.tests.unwrap_or_else(|| root.join("tests")), | |||
tests: self.tests.unwrap_or_else(|| root.join("test")), |
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.
I changed here, because it seems like tests
was a mistake here. Should I revert it?
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
@@ -420,7 +437,8 @@ impl Default for ProjectPaths { | |||
Self { | |||
artifacts: "out".into(), | |||
sources: "src".into(), | |||
tests: "tests".into(), | |||
tests: "test".into(), |
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.
that makes sense, is also the default of foundry::Config
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.
👍 👍
we know we can decode them properly by this point, they are flaky because people change their info
Motivation
Currently, our convention on foundry is to have scripts in the
script/
folder. However, the folder does not actually belong to the project. It may lead to weird issues, if there's contracts declared inside when verifying contracts.Alternatively, we could also add a more generic
additional_sources
where by default we add thescript/
folder.Solution
PR Checklist