Skip to content

chore: remove duplicate imports. #3306

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

Merged
merged 1 commit into from
May 6, 2025

Conversation

bznein
Copy link
Collaborator

@bznein bznein commented Apr 24, 2025

Inspired by #3298, I had ChatGPT come up with an ugly but working bash script that finds all duplicate imports. It looks like golangci doesn't have a linter for this specific.

Before asking for reviews, here is a check list of the most common things you might need to consider:

  • updating the Changelog
  • writing unit tests
  • checking if your changes affect other coins or tokens in unintended ways
  • testing on multiple environments (Qt, Android, ...)
  • having an AI review your changes

@bznein bznein requested a review from benma April 24, 2025 13:20
Copy link
Contributor

@benma benma left a comment

Choose a reason for hiding this comment

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

So where is the script? 😁 Could it be added to CI?

"github.com/BitBoxSwiss/bitbox-wallet-app/backend/bitsurance"
"github.com/BitBoxSwiss/bitbox-wallet-app/backend/coins/btc"
"github.com/BitBoxSwiss/bitbox-wallet-app/backend/coins/coin"
coinpkg "github.com/BitBoxSwiss/bitbox-wallet-app/backend/coins/coin"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in case of dups, you should keep the named one and remove the unnamed one. The reason for naming these imports is to avoid clashes or generic names. func foo(coin coin.Coin) is no good 😅

Not sure what the idiomatic way is, as I assume renaming imports is not very idiomatic. I am afraid it might be one letter var name like func foo(c coin.Coin), but I hate short names with a passion.

Copy link
Collaborator Author

@bznein bznein Apr 24, 2025

Choose a reason for hiding this comment

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

I think in case of dups, you should keep the named one and remove the unnamed one. The reason for naming these imports is to avoid clashes or generic names. func foo(coin coin.Coin) is no good 😅
Not sure what the idiomatic way is, as I assume renaming imports is not very idiomatic.

Exactly, according to this: https://google.github.io/styleguide/go/decisions#import-renaming imports shouldn't be renamed unless there are good reasons

The reason for naming these imports is to avoid clashes or generic names. func foo(coin coin.Coin) is no good 😅

I don't think this is solved by renaming the package import:
func foo (coin coinpkg.Coin) doesn't look much better imho. In general having a type Coin in a package coin will inevitably leads us to situations like these.

Copy link
Contributor

Choose a reason for hiding this comment

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

func foo (coin coinpkg.Coin) doesn't look much better imho

I should have been more specific. It's not the optics, it's about the name shadowing. Inside the foo function you can't use the coin package anymore b/c it's shadowed by the coin argument. It's a bit silly that vars and packages live in the same namespace in Go, but since it is, we should not shadow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd personally argue that it is better to rename the argument rather than the package (smaller scope, and it's easier to find a more meaningful name for an arg rather than a pkg) but I will not fight over it :D I'll make the changes!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a fight 😄 I think your point makes sense and I'm okay with it too.

@bznein
Copy link
Collaborator Author

bznein commented Apr 24, 2025

So where is the script? 😁 Could it be added to CI?

Well there's the script:

#!/bin/bash

find . -name "*.go" | while read -r file; do
  in_import_block=0
  imports=()

  while IFS= read -r line; do
    # Start of import block
    if [[ $line =~ ^[[:space:]]*import[[:space:]]*\( ]]; then
      in_import_block=1
      continue
    fi

    # End of import block
    if [[ $in_import_block -eq 1 && $line =~ ^[[:space:]]*\) ]]; then
      in_import_block=0
      continue
    fi

    # Single import line
    if [[ $line =~ ^[[:space:]]*import[[:space:]]+ ]]; then
      imp=$(echo "$line" | sed -E 's/.*"(.*)".*/\1/')
      imports+=("$imp")
      continue
    fi

    # Inside import block
    if [[ $in_import_block -eq 1 && $line =~ \" ]]; then
      imp=$(echo "$line" | sed -E 's/.*"(.*)".*/\1/')
      imports+=("$imp")
    fi

  done < "$file"

  # Now check for duplicates
  if [ ${#imports[@]} -gt 0 ]; then
    duplicates=$(printf "%s\n" "${imports[@]}" | sort | uniq -d)
    if [ -n "$duplicates" ]; then
      echo "In file $file:"
      echo "$duplicates" | while read -r dup; do
        echo "  Duplicate import: $dup"
      done
    fi
  fi
done

It can probably be added to the CI but I would rather write something non in bash :) easier to test and maintain

@bznein bznein force-pushed the duplicate_imports branch 2 times, most recently from c0b95f6 to c8fe216 Compare April 28, 2025 08:25
Copy link
Collaborator Author

@bznein bznein left a comment

Choose a reason for hiding this comment

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

@benma this is ready for review again. I changed it so that in case of duplicate imports, the unnamed one is removed and the aliased one is kept.

Let me know if you want me to write a proper linter/script that detects duplicate imports; I have some free cycles to work on it but I'd prefer to have it in a proper programming language :)

@bznein bznein requested a review from benma April 28, 2025 08:27
@@ -969,7 +967,7 @@ type scriptTypeWithKeypath struct {
keypath signing.AbsoluteKeypath
}

// adds a combined BTC account with the given script types.
// adds a combined BTC account with the given script accountsTypes.
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong search&replace

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed together with a few others, thanks for catching it!

@bznein bznein force-pushed the duplicate_imports branch from c8fe216 to 91fa2b6 Compare April 30, 2025 14:33
@bznein bznein requested a review from benma April 30, 2025 14:33
@bznein bznein merged commit 797af2f into BitBoxSwiss:master May 6, 2025
10 checks passed
@bznein bznein deleted the duplicate_imports branch May 6, 2025 08:18
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