Replace 'abuse limit' with 'secondary limit' by oscard0m · Pull Request #455 · octokit/plugin-throttling.js

oscard0m · 2022-02-12T16:58:50Z

Fixes #439

wolfy1339 · 2022-02-12T18:04:10Z

This would be a breaking change, let's deprecate the "abuse" named methods and add new secondary rate limit named methods.

We can then remove these in a new major version once the new team is in place.

oscard0m · 2022-02-13T19:41:35Z

src/index.ts

+      deprecate(
+        state.onAbuseLimit,
+        "'onAbuseLimit' is deprecated, use 'onSecondaryRateLimit' instead"
+      )


@wolfy1339
This is the method you expected to be deprecated right?

Is util/deprecate good in this case??

This is the method you expected to be deprecated right?

Yes

Is util/deprecate good in this case??

That's a NodeJS only function. Octokit can be run in a browser environment.
Take example from this commit:
octokit/webhooks.js@f8f3d15

Change applied.
For the @deprecated annotation I think it has sense to improve types for octokitOptions so we can mark it as deprecated. What do you think @wolfy1339 ?

Drafted a PR here: #457

oscard0m · 2022-02-13T19:43:16Z

src/index.ts

      minimumAbuseRetryAfter: 5,
+      minimumSecondaryRateRetryAfter: 5,


I decided to replace property minimumAbuseRetryAfter with minimumSecondaryRateRetryAfter. I assume the plugin is in control of it and it's not expected to be passed as an option by the user.

Is my assumption correct?

oscard0m · 2022-02-13T19:44:54Z

src/index.ts

  // @ts-ignore
-  events.on("abuse-limit", state.onAbuseLimit);
+  events.on(
+    "secondary-limit",


I'm assuming secondary-limit event will be sent from the L145 (so we have full control of the name). Is that correct or it should be listening to both (["abuse-limit", "secondary-limit"]?

test/events.test.ts

…mit'

docs(secondary-limits): replace 'abuse-limits' with 'secondary-limits'

wolfy1339

Looks good to me

gr2m · 2022-03-02T19:43:56Z

you merged it to master, but no new release was created, because semantic-release didn't understand the commit. I think this is a breaking change, right? Ideally we would keep supporting the previous APIs, but log a warning. Maybe that's something we could still do, to avoid a breaking change? I understand if you don't have the time for that though, releasing a breaking change is fine

Sorry for the late response, I don't have much time right now to help with Octokit 😫

oscard0m · 2022-03-02T21:59:50Z

you merged it to master, but no new release was created, because semantic-release didn't understand the commit.

Interesting. I'm going to take a look on why that happened. Commits look good to me in terms of semantic.

I think this is a breaking change, right? Ideally we would keep supporting the previous APIs, but log a warning. Maybe that's something we could still do, to avoid a breaking change?

I think we did the console.warn logic already. Maybe we missed something else you were expecting?

I understand if you don't have the time for that though, releasing a breaking change is fine

Once a week I have time to contribute to OSS. I have time tomorrow CET so... no problem 😉. I'm here to help and learn.

Sorry for the late response, I don't have much time right now to help with Octokit 😫

No worries at all. Whenever you have time is fine, no rush.

gr2m · 2022-03-02T22:28:46Z

Commits look good to me in terms of semantic.

when you use "Squash & Merge", then the commit message defaults to the pull request title, if there is more than one commit.

I think we did the console.warn logic already. Maybe we missed something else you were expecting?

oh okay if you already have deprecation than it's all good. I think I usually use octokit.log.warn, I try to avoid using console. That way people can use their own log logic

oscard0m · 2022-03-02T23:12:25Z

when you use "Squash & Merge", then the commit message defaults to the pull request title, if there is more than one commit.

True. What's the process when we commit this mistake?

oh okay if you already have deprecation than it's all good. I think I usually use octokit.log.warn, I try to avoid using console. That way people can use their own log logic

Since we need to fix the commit message + release I can include octokit.log.warn

gr2m · 2022-03-03T00:10:56Z

when you use "Squash & Merge", then the commit message defaults to the pull request title, if there is more than one commit.

True. What's the process when we commit this mistake?

Good question, we should document that, probably at https://github.com/octokit/octokit.js/blob/main/CONTRIBUTING.md#maintainers-only?

I would create an empty commit with the correct commit message & body, that will trigger the release without making any code changes

github-actions · 2022-03-03T18:59:47Z

🎉 This PR is included in version 3.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

GitHub renamed `abuse limits` to `secondary rate limits` in 2021: https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits `@octokit/plugin-throttling` followed suit in March 2022, and released version `3.6.0` with this new name (and deprecated `onAbuseLimit`): octokit/plugin-throttling.js#455 This commit updates the README file to use the new name.

GitHub renamed `abuse limits` to `secondary rate limits` in 2021: https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits `@octokit/plugin-throttling` followed suit in March 2022, and released version `3.6.0` with this new name (and deprecated `onAbuseLimit`): octokit/plugin-throttling.js#455 This commit updates the codebase to use the new name.

oscard0m requested a review from gr2m February 12, 2022 16:58

oscard0m added the Type: Maintenance Tests, Refactorings, Automation, etc label Feb 12, 2022

oscard0m force-pushed the replace-abuse-limit-with-secondary-limit branch from 97330ae to 4f81504 Compare February 13, 2022 18:57

oscard0m requested a review from wolfy1339 February 13, 2022 18:58

oscard0m marked this pull request as draft February 13, 2022 19:09

oscard0m force-pushed the replace-abuse-limit-with-secondary-limit branch 3 times, most recently from e45018d to a36302d Compare February 13, 2022 19:49

oscard0m commented Feb 13, 2022

View reviewed changes

oscard0m force-pushed the replace-abuse-limit-with-secondary-limit branch 2 times, most recently from 4231afb to 7241f93 Compare February 18, 2022 14:14

oscard0m added 3 commits February 18, 2022 15:33

feat(secondary-limit): replace 'abuse-limit' logic with 'secondary-li…

a1e9c3d

…mit'

test(secondary-limit): replace 'abuse-limit' with 'secondary-limit'

f7ed271

docs(secondary-limit): replace 'abuse-limits' with 'secondary-limits'

872d4a1

docs(secondary-limits): replace 'abuse-limits' with 'secondary-limits'

oscard0m force-pushed the replace-abuse-limit-with-secondary-limit branch from 7241f93 to 872d4a1 Compare February 18, 2022 14:33

oscard0m marked this pull request as ready for review February 18, 2022 14:34

wolfy1339 approved these changes Feb 26, 2022

View reviewed changes

oscard0m merged commit 4c45c2e into master Mar 2, 2022

oscard0m deleted the replace-abuse-limit-with-secondary-limit branch March 2, 2022 11:29

github-actions bot added the released label Mar 3, 2022

nimadini mentioned this pull request Aug 4, 2022

fix: use "secondary" instead of "abuse" for rate limit octokit/octokit.js#2266

Merged

		minimumAbuseRetryAfter: 5,
		minimumSecondaryRateRetryAfter: 5,

Conversation

oscard0m commented Feb 12, 2022

Uh oh!

wolfy1339 commented Feb 12, 2022

Uh oh!

oscard0m Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

wolfy1339 Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

oscard0m Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

oscard0m Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

oscard0m Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscard0m Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wolfy1339 left a comment

Choose a reason for hiding this comment

Uh oh!

gr2m commented Mar 2, 2022

Uh oh!

oscard0m commented Mar 2, 2022

Uh oh!

gr2m commented Mar 2, 2022

Uh oh!

oscard0m commented Mar 2, 2022

Uh oh!

gr2m commented Mar 3, 2022

Uh oh!

github-actions bot commented Mar 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oscard0m Feb 13, 2022 •

edited

Loading