-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Conversation
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.
This seems like a definite improvement on the out-of-date file we have currently. Thanks again!
Thanks @swisstackle! This is much easier to review! The Previous PR's merges required a level of focus I save for well caffeinated times :) Please note, since this PR updates a file that rarely gets touched (previously 19-Feb-2020), you wouldn't need to pull in commits from up-stream. But if you do, please consider |
Yes. Also, I think the issues happened because I didnt create my own branch locally (to then rebase it and attach it to the newest and updated codebase). Ill checkout your link. |
@cds-amal In the article about rerere, the author used git merge to make the test merges. But let's say I have a scenario where Id have to actually merge the changes into the feature branch, then I would use rebase instead right (to avoid the annoying inbetween merge commits) Thanks for that article! |
Yes. The conflict resolution issues are the same for rebase and merge. A rebased feature branch is a lot easier to reason about, and review! |
Once we figured everything out for Windows user, we maybe should also consider adding some info about that, but I guess for now it's sufficient. |
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.
Thanks for this @swisstackle! It is certainly a better guide to contribution. From your perspective as a first-time contributor, is there anything else we can do better to encourage and help new contributors?
``` | ||
|
||
### Create a new command in `@truffle/core` | ||
|
||
Create a new file in `packages/core/lib/commands/`, let's call it `mycmd.js`. | ||
1. Create a new directory in `packages/core/lib/commands/`, let's call it `mycmd`. | ||
2. Create 3 javascript files inside the **mycmd** directory with the filenames **run.js**, **meta.js** and **index.js**: |
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.
A new command needs to be explicitly listed in this index.js as per #5114. Maybe that file should have a comment to that point? @eggplantzzz?
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.
We should figure out a better way to consolidate these so we don't need to maintain this list in two places. Maybe we can just get rid of the index file you mentioned. Buuuuuutt, that is for another day and PR.
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.
@swisstackle I'm delighted you brought attention to 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.
@cds-amal I believe that that is already mentioned in the file:
Link it from the commands/index.js file
--- packages/core/lib/commands/index.js
+++ packages/core/lib/commands/index.js
@@ -1,4 +1,5 @@
module.exports = {
+ mycmd: require("./mycmd"),
However there is also the packages/core/lib/commands/commands.js file in the same directory. Do we need to write it in there as well?
Update: @eggplantzzz mentioned it. Nevermind. Ill add 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.
Yeah, commands.js
is a file that got recently added when making optimizations to Truffle.
Small typo
@cds-amal "Thanks for this @swisstackle! It is certainly a better guide to contribution. From your perspective as a first-time contributor, is there anything else we can do better to encourage and help new contributors?" It's really just the barriers for Windows users and this file that were a problem. But persistent people like me will always figure it out, but not everybody is persistent. I know about an onboarding software called "Codesee" which let's you create repository tours/maps/graphs for new employees/interns or other contributors. Maybe that makes it easier to understand the codebase in general. I played around with it a little bit and I thought it was a good product. |
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.
Looks good to me! Thanks for this @swisstackle
Yes, we have to improve this for Windows users and contributors. There's an effort to improve this.
I haven't heard of this and will look in to it! I noticed codesee files in the previous, closed commit. Were you able to utilize it to help understand Truffle's code base? |
this PR addresses #5144 |
@cds-amal I was able to utilize Codesee a bit for truffle. However, I think the full potential will only be reached if you create a "repository tour" for your contributors that will tour them through the repository and explain everything in the correct order. But the graph by itself helped seeing dependencies between code files. |
There we go.
Let me know if anything else has to be changed.