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

refactor: singleton octokit instance for shared throttling state #640

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gr2m
Copy link
Member

@gr2m gr2m commented May 29, 2023

No description provided.

@gr2m gr2m changed the base branch from beta to beta-no-more-serial May 30, 2023 00:03
@gr2m gr2m changed the title WIP singleton octokit instance refactor: singleton octokit instance for shared throttling state May 30, 2023
@gr2m gr2m changed the title refactor: singleton octokit instance for shared throttling state refactor: singleton octokit instance for shared throttling state May 30, 2023
Base automatically changed from beta-no-more-serial to beta May 30, 2023 02:53
Copy link
Member

@travi travi left a comment

Choose a reason for hiding this comment

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

i like where this is headed. only comments are calling out most of your todos. am i correct to assume that is most of what remains for this to still be draft, or do you have additional plans?

@@ -199,7 +196,8 @@ test.serial("Update a prerelease", async (t) => {
t.true(fetch.done());
});

test.serial("Update a release with a custom github url", async (t) => {
// TODO: move to integration tests
Copy link
Member

Choose a reason for hiding this comment

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

is this a change that you are wanting to include in this PR?

@@ -345,7 +340,8 @@ test("Publish a release with one asset", async (t) => {
t.true(fetch.done());
});

test("Publish a release with one asset and custom github url", async (t) => {
// TODO: move to integration tests
Copy link
Member

Choose a reason for hiding this comment

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

is this a change you are wanting to include in this PR?

@@ -655,7 +647,8 @@ test("Publish a draft release with one asset", async (t) => {
t.true(fetch.done());
});

test("Publish a release when env.GITHUB_URL is set to https://github.com (Default in GitHub Actions, #268)", async (t) => {
// TODO: move to integration test
Copy link
Member

Choose a reason for hiding this comment

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

is this a change you are wanting to include in this PR?

t.true(fetch.done());
});

// TODO: move to integration test
Copy link
Member

Choose a reason for hiding this comment

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

is this a change you are wanting to include in this PR?

@travi
Copy link
Member

travi commented Jun 2, 2023

are we wanting to include this refactor in the ESM change, or follow up with it afterward? i've had no problems in my testing with the current beta that includes the ESM change for our two candidate plugins, so wondering if we promote those candidates or wait for this change to go in

@gr2m
Copy link
Member Author

gr2m commented Jun 2, 2023

only comments are calling out most of your todos. am i correct to assume that is most of what remains for this to still be draft

correct

are we wanting to include this refactor in the ESM change, or follow up with it afterward

I planned to give this another go today but I won't have time. I'd say let's go ahead with the ESM change and then do this afterwards.

Base automatically changed from beta to master June 2, 2023 19:15
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