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

chore: sort imports #179

Merged
merged 2 commits into from
Jul 24, 2024
Merged

chore: sort imports #179

merged 2 commits into from
Jul 24, 2024

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented Jul 23, 2024

Description

Today I learned about Foundry's sort_imports fmt configuration option.

As it has almost zero dev cost to use it, I'd go for it.

Drawback: I noticed a bug while formatting our code, specifically this section:

import {IERC20} from "src/contracts/interfaces/IERC20.sol";
import {GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol";
import {GPv2Order} from "src/contracts/libraries/GPv2Order.sol";
import {GPv2Trade} from "src/contracts/libraries/GPv2Trade.sol";

was changed to:

import {IERC20} from "src/contracts/interfaces/IERC20.sol";

import {GPv2Order} from "src/contracts/libraries/GPv2Order.sol";
import {GPv2Trade} from "src/contracts/libraries/GPv2Trade.sol";
import {GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol";

and I had to manually remove the extra newline.
I'd still say this isn't a big issue as we'd notice on reviewing.

Since I noticed it, I also removed a duplicate import.

Test Plan

CI doesn't fail on fmt --check.

@fedgiac fedgiac requested a review from a team as a code owner July 23, 2024 16:57
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;

import {GPv2Transfer, IERC20, IVault, GPv2Transfer, GPv2Order} from "src/contracts/GPv2VaultRelayer.sol";
import {GPv2Order, GPv2Transfer, GPv2Transfer, IERC20, IVault} from "src/contracts/GPv2VaultRelayer.sol";
Copy link
Contributor Author

@fedgiac fedgiac Jul 23, 2024

Choose a reason for hiding this comment

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

It also makes duplicated imports like this easier to spot. (linter issue for adding a related rule here.)

mfw78
mfw78 previously approved these changes Jul 24, 2024
Copy link
Contributor

@mfw78 mfw78 left a comment

Choose a reason for hiding this comment

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

Also can remove the duplicate import prior to merging

@fedgiac fedgiac merged commit 239429f into main Jul 24, 2024
10 checks passed
@fedgiac fedgiac deleted the sort-imports branch July 24, 2024 09:52
@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants