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: remove stdlib errors package from lint blocklist #9381

Merged
merged 1 commit into from
May 28, 2024

Conversation

maxrussell
Copy link
Contributor

Ticket

Description

We've had the stdlib errors package disallowed since at least April of 2020. More recently, we decided to stop using github.com/pkg/errors, which is a good thing IMO. But we need the stdlib errors for stuff like errors.Is, so this removes it from the list of disallowed packages.

Test Plan

None, it's a linting/CI change.

@maxrussell maxrussell requested a review from a team as a code owner May 16, 2024 01:02
@cla-bot cla-bot bot added the cla-signed label May 16, 2024
Copy link

netlify bot commented May 16, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 1c16466
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66565562c8234f0008e242ba

Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.60%. Comparing base (515c135) to head (1c16466).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9381      +/-   ##
==========================================
- Coverage   48.60%   48.60%   -0.01%     
==========================================
  Files        1233     1233              
  Lines      158981   158981              
  Branches     2778     2777       -1     
==========================================
- Hits        77271    77267       -4     
- Misses      81536    81540       +4     
  Partials      174      174              
Flag Coverage Δ
backend 42.14% <ø> (-0.01%) ⬇️
harness 64.02% <ø> (ø)
web 44.38% <ø> (ø)

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

see 3 files with indirect coverage changes

Copy link
Contributor

@stoksc stoksc left a comment

Choose a reason for hiding this comment

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

cool. i've wanted to do his, ban pkg/errors, and migrate from errors.Wrap to fmt.Errorf for a while.

@maxrussell maxrussell enabled auto-merge (squash) May 16, 2024 01:13
Copy link
Contributor

@NicholasBlaskey NicholasBlaskey left a comment

Choose a reason for hiding this comment

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

can we first migrate off pkg/errors so we don't have two errors imported in every file?

@maxrussell maxrussell disabled auto-merge May 16, 2024 22:03
@maxrussell
Copy link
Contributor Author

@NicholasBlaskey: it's an option, sure. I'd rather stuff import both packages than expand usage of pkg/errors, but I'm open to having my mind changed.

@stoksc
Copy link
Contributor

stoksc commented May 17, 2024

Letting people use errors now instead of github.com/pkg/errors is going to be less work once we do ban the latter. I doubt many folks will import errors into a file already using github.com/pkg/errors in the meantime.

@maxrussell maxrussell enabled auto-merge (squash) May 22, 2024 23:55
@NicholasBlaskey
Copy link
Contributor

@NicholasBlaskey: it's an option, sure. I'd rather stuff import both packages than expand usage of pkg/errors, but I'm open to having my mind changed.

Is there anything blocking us from banning pkg/errors and swapping it over now? Can we just go ahead and do it?

Copy link
Contributor

@NicholasBlaskey NicholasBlaskey left a comment

Choose a reason for hiding this comment

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

lgtm sorry I took awhile to respond this

I'm not against this change, just think getting off pkg/errors is more important

@maxrussell
Copy link
Contributor Author

Is there anything blocking us from banning pkg/errors and swapping it over now? Can we just go ahead and do it?

Not that I'm aware of other than time. grep tells me there's 218 instances of errors.New in the repo, which is fewer than I would have guessed but still more than I want to address at the moment.

I'm not against this change, just think getting off pkg/errors is more important

@NicholasBlaskey Totally hear that and agree. I just wanted to use stdlib errors and was blocked by an out-of-date linting rule and wanted to fix it and move on. I fully support swapping over and blocking pkg/errors and think it deserves somebody's tech debt time.

@maxrussell maxrussell merged commit 9abde37 into main May 28, 2024
81 of 96 checks passed
@maxrussell maxrussell deleted the max/stop-stopping-errors branch May 28, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants