Skip to content
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

Add warning for deleted files to prevent errors during processing #163

Merged
merged 10 commits into from
Dec 7, 2024

Conversation

moltam89
Copy link

@moltam89 moltam89 commented Dec 3, 2024

Hey guys,

I was working on an extension, where I deleted the default YourContract.sol file, which resulted in an error when I tried to use yarn create-extension {projectName}.

Steps to reproduce:

  1. yarn cli
  2. Delete e.g. the YourContract.sol file
  3. yarn create-extension {projectName}

Screenshot 2024-12-03 at 17 02 20

After this fix it should work like this:

Screenshot 2024-12-03 at 17 08 10

@technophile-04
Copy link
Collaborator

technophile-04 commented Dec 4, 2024

Hey thanks @moltam89! actually, the real problem was that since we were finding diff earlier like:

const { stdout } = await execa("git", ["diff", "--name-only", `${firstCommit.trim()}..HEAD`], {
    cwd: projectPath,
});

Timeline: 1. initial commit (had YourContract.sol), 2. In second commit you removed YourContract.sol)

Now because of the above git logic. Even though you removed YourContract.sol in second commit the above result of stdout would still contain YourContract.sol. We have to pass --diff-filter=d to tell git exclude deleted files.


Another improvement I pushed to create-extension CLI is let suppose, Timeline:

  1. Initial commit
  2. Createa file and add it as commit (In this example I created this commit adding a new page)
  3. I run yarn create-extension <instance_path> (This will copy newly created page in cli/externalExtension/<instance_name>
  4. I delete that page from instance, commit and now I run create-extension command again. This will delete the file from cli/externalExtension/<instance_name> since extension developer don't want this file anymore in his extension.
Test instructions:
  1. Clone my instance outside of this CLI repo.
git clone https://github.com/technophile-04/ctf-extension-instance.git && cd ctf-extension-instance
  1. Now we switch the instance to where we added a new page
git checkout c6eb90c666cdfee52a31ca969fb9ee62864066c5
  1. We go to CLI repo and run it to generate the extension.
yarn build:dev && yarn create-extension ../ctf-extension-instance

^ This will create a new page in extensionExtension/ctf-extension-instance

  1. Now in Clonned instance repo we switch to main commit where we deleted the newly added page.
git checkout main
  1. In CLI repo we again run the command it shall delete he deleted page from extension too.
yarn create-extension ../ctf-extension-instance

@moltam89
Copy link
Author

moltam89 commented Dec 4, 2024

Thanks @technophile-04 ,

This is a thorough solution, works great for me!

Copy link
Member

@rin-st rin-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Shiv, great PR!

Working great to me! But added a bunch of nitpicks

src/dev/create-extension.ts Outdated Show resolved Hide resolved
src/dev/create-extension.ts Outdated Show resolved Hide resolved
src/dev/create-extension.ts Show resolved Hide resolved
src/dev/create-extension.ts Outdated Show resolved Hide resolved
src/dev/create-extension.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rin-st for the review just pushed the tweak 🙌

@rin-st
Copy link
Member

rin-st commented Dec 5, 2024

Thanks @rin-st for the review just pushed the tweak 🙌

Thank you! Lgtm!

Thank you Shiv, great PR!

Oops, @moltam89 thank you for idea and PR too!

@technophile-04 technophile-04 merged commit 16e6a9e into scaffold-eth:main Dec 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants