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

feat: don't close page before calling errorHandler #1548

Merged
merged 1 commit into from
Sep 19, 2022
Merged

Conversation

szmarczak
Copy link
Contributor

No description provided.

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

should this resolve #947 or do we need something else too? i dont see the closes #... here

@szmarczak
Copy link
Contributor Author

Depends on whether it includes errorHandler to be called on each error instead of retry. I'm wondering if that would be breaking.

Otherwise feel free to add the Closes.

@B4nan
Copy link
Member

B4nan commented Sep 19, 2022

Depends on whether it includes errorHandler to be called on each error instead of retry. I'm wondering if that would be breaking.

No plans to release that as breaking, it will be a patch bump, and if its not there, please do so (ideally it should be separate PR so we have separate line in changelog).

@szmarczak szmarczak merged commit 1c8cd82 into master Sep 19, 2022
@szmarczak szmarczak deleted the error-improve branch September 19, 2022 17:09
@B4nan
Copy link
Member

B4nan commented Sep 19, 2022

I was expecting that separate PR first, and tagging this issue with closes #..., before we merge it, so we have it propertly written in the changelog. We need to care more about what we merge and how the git history in master look like, as it will directly affect the changelog once we fully automate the releasing process.

@szmarczak
Copy link
Contributor Author

I was expecting that separate PR first

Why is that?

tagging this issue with closes #...

It doesn't make sense to close the issue if the other future PR is also a part of that issue, no?

@B4nan
Copy link
Member

B4nan commented Sep 19, 2022

This PR is technically fixing #947, isnt it? But it wont be linked in the generated changelog to this issue, that's why.

It doesn't make sense to close the issue if the other future PR is also a part of that issue, no?

Exactly, and that is why I was expecting the fix to come first. It will be a one liner, or no?

Anyway, not a big deal, and it does not matter much as its merged already. My main message was - let's try to think more about how the master git history looks like, as it will be used for generated changelog. And we want to link issues in the changelog description.

@szmarczak szmarczak mentioned this pull request Oct 10, 2022
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.

3 participants