-
Notifications
You must be signed in to change notification settings - Fork 135
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
refactor: move crates into crates folder and remove redundant prefixes #1624
refactor: move crates into crates folder and remove redundant prefixes #1624
Conversation
c4df32f
to
0dd6390
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.
I'm 👍 for the direction of this pr, but I also think this needs a 👍 from everyone before merging
Cargo.toml
Outdated
"crates/rpc", | ||
"crates/beacon", | ||
"crates/evm", | ||
"crates/history", |
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.
nitpick ... maybe we could leave the prefix on the history
/ state
/ beacon
crates? Sometimes when you have to make a change to one of these, you typically have to replicate it across all three, so it's nice when they're grouped together alphabetically
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 was thinking of putting them in a folder
so
crates/subnetworks/history
crates/subnetworks/state
crates/subnetworks/history
What do you think? I am open to ideas, I am fine doing the suggestion above, I think a folder would be a little cleaner though
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, works for me!
17a810b
to
fd1509f
Compare
the book ci is failing here is a PR to fix it #1628 |
fd1509f
to
e4fa0f2
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.
I'm OK with these changes, but as Nick suggested, everyone from the team needs to review this.
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 still see the ethportal-api/src/test_utils
folder in the root folder on my GitHub review dashboard, could you confirm that the whole ethportal-api folder is removed from the root?
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.
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, maybe, It looks good now.
e4fa0f2
to
e8eb455
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
I like this change 🚢
e8eb455
to
a9c7769
Compare
a9c7769
to
1ebb006
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.
Discussed live. GTG with follow-ups to match crate names to paths, and have consistent crate names.
What was wrong?
Both https://github.com/paradigmxyz/reth and https://github.com/lambdaclass/ethrex
use this general structure, I think it is very clean and makes it easier to expand our project going forward
How was it fixed?
moving folders, renaming a few as well
To-Do in a future PR
I am leaving
I want to move
utp-testing
andethportal-peertests
to the testing folder added in #1621But #1621 would need to be merged first so that will probably happen in a follow up PR