Skip to content

Wait for active pushing operations to finish before opening PR #4340

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

Conversation

ChrisMcD1
Copy link
Contributor

One thing I very much dislike about this PR is the interface of sync.Map. I was shocked to learn there wasn't a standard lib implementation with generics yet. https://github.com/puzpuzpuz/xsync seems to be what people have standardized around for a thread safe Map with actual types instead of any, but I didn't want to suggest adding that for a single map. If people like the library though, I'm happy to add it in!

I tested this with a pre-push hook that just did a sleep 3 to simulate extra-slow pushes. I changed the non-input open pull requests to happen on a background worker thread, and remember their commit has for when they need to retrieve it. In the case of needing user input, I figured it would be better to block the UI so that we don't have a popup show up several seconds later after the keystroke.

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

Comment on lines -56 to -58
if options.Mode == types.ASYNC && options.Then != nil {
panic("RefreshOptions.Then doesn't work with mode ASYNC")
}
Copy link
Contributor Author

@ChrisMcD1 ChrisMcD1 Feb 28, 2025

Choose a reason for hiding this comment

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

I'm not sure why the async options.Then wasn't always included in this function. Feels pretty natural.

I know I'm probably missing something though!

@ChrisMcD1 ChrisMcD1 force-pushed the track-active-pushing-operations branch 3 times, most recently from 14467cd to 7784840 Compare February 28, 2025 21:41
@ChrisMcD1 ChrisMcD1 force-pushed the track-active-pushing-operations branch from 7784840 to f2b0f2c Compare February 28, 2025 21:51
@ChrisMcD1 ChrisMcD1 marked this pull request as ready for review February 28, 2025 21:51
@ChrisMcD1
Copy link
Contributor Author

Closing because I have a superior implementation WIP, and it'll require different PRs that involve the gocui fork.

@ChrisMcD1 ChrisMcD1 closed this Mar 4, 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.

1 participant