Vendor forge-fmt into solang repo #1649
Conversation
|
I don't know if this is much of an effort, but have you considered adding forge-fmt as a submodule or as a git subtree? |
It needs modifications to make it work, and I don't know that that is possible with submodule or subtree. Having said that, if it could work it would be nice. |
| @@ -0,0 +1,22 @@ | |||
| [package] | |||
| name = "forge-fmt" | |||
| version = "0.2.0" | |||
There was a problem hiding this comment.
What version should it have if we rename it to solang-fmt?
There was a problem hiding this comment.
If we are going to publish it, intuitively should be the same version as solang-parser and solang?
There was a problem hiding this comment.
However if we just want to vendor it we should keep the original version I think
There was a problem hiding this comment.
How about we just vendor it for now. If we want to publish it, we can always rename it and give it a new version.
| @@ -0,0 +1,22 @@ | |||
| [package] | |||
| name = "forge-fmt" | |||
There was a problem hiding this comment.
I'm thinking, should we name it solang-fmt instead? Or do we just copy it over from time to time leaving as much as possible as it is?
There was a problem hiding this comment.
It's a pretty useful crate TBH. We might want to publish it in the future.
So I agree, why not rename it to solang-fmt.
There was a problem hiding this comment.
I've changed my mind. Let's just vendor it for now.
| version = "0.2.0" | ||
| edition = "2021" | ||
| authors = ["Foundry Contributors"] | ||
| license = "MIT OR Apache-2.0" |
There was a problem hiding this comment.
Is this compatible with our license (looks like we have Apache only)?
| homepage = "https://github.com/foundry-rs/foundry" | ||
| repository = "https://github.com/foundry-rs/foundry" |
There was a problem hiding this comment.
Is this intentional? Might be misleading for example if we want to publish it on crates.io but I guess this isn't supposed to be published?
There was a problem hiding this comment.
As we're vendoring for now, let's leave it as is
|
Yeah I quickly had the same thought as @LucasSte, however we'd have to modify it anyways and it wouldn't be just |
|
@seanyoung also noticed the PR title could be renamed to "Vendor forge-fmt into solang" as it would explain the reasoning of why we need this :) |
seanyoung
changed the title
The published forge-fmt is out date, not maintained and depends on an old version of solang-parser. This is a copy of forge-fmt: https://github.com/foundry-rs/foundry/tree/master/crates/fmt commit: 00854b602ef0e67379a2027ccc5d0aad553e5333 Some minor modifications were needed to make it work with the new parse tree. Signed-off-by: Sean Young <[email protected]>
seanyoung
changed the title
|
The windows failure is unrelated to this PR |
The published forge-fmt is out date, not maintained and depends on an old version of solang-parser.
This is a copy of forge-fmt:
https://github.com/foundry-rs/foundry/tree/master/crates/fmt
commit: 00854b602ef0e67379a2027ccc5d0aad553e5333