-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(config): add config crate #297
Conversation
I'm +1 on this design, plus other things like compiler optimizations, enabling of FFI etc. |
This is already looking really good. One thing we should do is:
|
sgtm! traversing dirs is built in directly to |
9c4685a
to
7431852
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 so far, minor nits. Just need to:
- add it in
forge init
- add some docs about it in the top-level README, to guide ppl to start creating these config files in their dev envs
- change the root dir detection from being the .git dir, to the first directory
foundry.toml
is found in OR the .git dir for a project.
Updates
|
Todo
|
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 - wonder if we should combine the new Remapping struct with the old one?
optimizer = false | ||
optimizer_runs = 200 | ||
verbosity = 0 | ||
ignored_error_codes = [] |
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.
think we should at a minimum have a list of the error codes somewhere, and what they correspond to? https://gist.github.com/onbjerg/a7d1243bf1f4c795edace88aa35fff3a
https://gist.github.com/mattsse/a2b6c5f894532815e8e5a210fe87ddd1
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.
yeah, seems like a good thing to have in ethers-solc, perhaps there is way to figure out what error codes are propagated to solc, will take another look at solidity codebase
Not sure if it's out of scope for this PR, but should we also have something like |
I'd pull all of these Configs in the Foundry.toml, instead of having many configs |
yeah we can support a |
Update
Todo
cc @gakonst |
blocked by gakonst/ethers-rs#810 added more integration tests and docs. Behavior of remappings is currently different than on master: remappings are not extended but replaced as a whole: Remappings are built in this order (last item takes precedence)
|
/// |__ cwd | ||
/// ``` | ||
/// will still detect `repo` as root | ||
pub fn find_project_root_path() -> eyre::Result<PathBuf> { |
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.
amazing
closes #291
Adds a config crate, see
README
for description