Skip to content

Move the signature hash errors into the same file#748

Closed
philipch07 wants to merge 1 commit intomasterfrom
pch07/move-signature-hash-errors-into-same-file
Closed

Move the signature hash errors into the same file#748
philipch07 wants to merge 1 commit intomasterfrom
pch07/move-signature-hash-errors-into-same-file

Conversation

@philipch07
Copy link
Copy Markdown
Contributor

Description

The errors in the signature hash module were in a separate file. I'm assuming it's related to an older version of go. It can be condensed into the same file so we don't have an extra file just to define 4 errors.

Reference issue

N/A

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.13%. Comparing base (7b68bd9) to head (db7af7a).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #748   +/-   ##
=======================================
  Coverage   79.13%   79.13%           
=======================================
  Files         102      102           
  Lines        6917     6917           
=======================================
  Hits         5474     5474           
  Misses       1068     1068           
  Partials      375      375           
Flag Coverage Δ
go 79.16% <ø> (ø)
wasm 58.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

I'm not sure about this change, we use the errors.go pattern a lot for errors in the same package, yes it's not required if we have a single file in the package, but I'm not sure if we should change if we already did it, it's nice to have a place for all the errors defined by a package.

Go doesn't care about files in the same package, it's all the same scope.

@philipch07
Copy link
Copy Markdown
Contributor Author

I'm not sure about this change, we use the errors.go pattern a lot for errors in the same package, yes it's not required if we have a single file in the package, but I'm not sure if we should change if we already did it, it's nice to have a place for all the errors defined by a package.

That's true. I think this PR is addressing the edge case since there's only really one file in this package. It doesn't really matter all that much though, perhaps it's easiest to keep things as is unless things get changed up more. Going forward, do you have a suggestion for whether we should still follow the errors.go pattern even if a package only has one file, or would you prefer for the errors to be defined at the top of the one file?

Go doesn't care about files in the same package, it's all the same scope.

Yes, this makes sense.

@JoTurk
Copy link
Copy Markdown
Member

JoTurk commented Oct 17, 2025

That's true. I think this PR is addressing the edge case since there's only really one file in this package. It doesn't really matter all that much though, perhaps it's easiest to keep things as is unless things get changed up more. Going forward, do you have a suggestion for whether we should still follow the errors.go pattern even if a package only has one file, or would you prefer for the errors to be defined at the top of the one file?

I was thinking actually to enforce the errors.go pattern in the future regardless.

@JoTurk
Copy link
Copy Markdown
Member

JoTurk commented Oct 17, 2025

Maybe using a lint rule (extreme), what do you think?

@philipch07
Copy link
Copy Markdown
Contributor Author

I was thinking actually to enforce the errors.go pattern in the future regardless.

Sounds good to me!

Maybe using a lint rule (extreme), what do you think?

If you think it's a good idea, I don't mind going for it. It would be a pretty big change though, so perhaps we should get other people's opinions on this, or maybe check out other large codebases to get a feel for what they do and perhaps if they have any rationale. If you do want to move ahead with the lint rule, just let me know and I'll help to reorganize everything! :)

@philipch07 philipch07 closed this Oct 17, 2025
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