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

Fixing edge case when import files with same contract name at least two times #420

Conversation

Ratimon
Copy link

@Ratimon Ratimon commented Dec 13, 2024

There is an edge case as following:

Suppose I would like to accomplish following code block:

pragma solidity ^0.8.20;

import "@redprint-core/cannon/libraries/CannonErrors.sol";
import "@redprint-core/cannon/libraries/CannonTypes.sol";

and I use :

    const CannonErrors = {
        name: '',
        path: '@redprint-core/cannon/libraries/CannonErrors.sol',
    };
    c.addImportOnly(CannonErrors);
    const CannonTypes = {
        name: '',
        path: '@redprint-core/cannon/libraries/CannonTypes.sol',
    };    
    c.addImportOnly(CannonTypes);

However, it will generate only one path with excessive {} from as following :

/ SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {} from "@redprint-core/cannon/libraries/CannonErrors.sol";

Adding additional state of readonly modules: ImportContract[] = []; with modified contract.ts and print.ts 's functions as PR will solve this:

Please suggest if this is not appropriate, I am happy to make another PR.

Reference to : my repo

Regards

@Ratimon Ratimon changed the title Fixing edge case when import files with '' contract name at least two times Fixing edge case when import files with same contract name at least two times Dec 13, 2024
@ericglau
Copy link
Member

Hi, thanks for the PR. However, this code is internal logic and not part of the Contracts Wizard's external API. We currently do not have a need for importing entire Solidity files in the output that the Contracts Wizard generates, and this was an intentional change to use named imports in #411. As such, I don't think it is appropriate to add code paths that we do not use.

The recommended best practice is to use named imports, and you can see this pattern in use in OpenZeppelin Contracts, and this is also recommended in style guides such as Coinbase's style guide.

As your project uses a fork of this code, it looks like you already have the changes that you need in your repo already, so it does not depend on this PR. I will close this PR, but feel free to continue the conversation if needed. Thanks.

@ericglau ericglau closed this Dec 13, 2024
@Ratimon
Copy link
Author

Ratimon commented Dec 13, 2024

Thanks for the review!!

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.

2 participants