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

Add type declaration to MigrationCollector::reset() #491

Merged
merged 9 commits into from Mar 23, 2023

Conversation

asuri0n
Copy link
Contributor

@asuri0n asuri0n commented Mar 14, 2023

Added the explicit @return type in MigrationCollector::reset() to suspend the deprecation info msg

image

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Please use a meaningful commit message as described in our contribution guide.

@asuri0n
Copy link
Contributor Author

asuri0n commented Mar 20, 2023

@SenseException I don't know how to edit my previous commit messages

@stof
Copy link
Member

stof commented Mar 20, 2023

you need to amend your git history locally and to force-push your branch after that

@SenseException
Copy link
Member

SenseException commented Mar 22, 2023

@asuri0n You can find infos on how to change the last commit here: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History#_git_amend and do a force-push afterwards.

@asuri0n
Copy link
Contributor Author

asuri0n commented Mar 22, 2023

well ... I think I screwed up the git history.... I will make a new merge request with clean commit message.

@greg0ire
Copy link
Member

greg0ire commented Mar 22, 2023

I will make a new merge request with clean commit message.

You don't need to, you can just force push once the history is good.

What you could do is

git reset 9432bc37ab3bc4f074425c669d8a63cc388b17df
git add --patch
git commit --verbose
git push --force-with-lease

@greg0ire
Copy link
Member

Cc @mpdude, regarding doctrine/doctrine-website#473 (comment) :)

@greg0ire
Copy link
Member

@asuri0n there are 9 commits now. Have you tried what I recommended above?

@mpdude
Copy link

mpdude commented Mar 22, 2023

If we‘d ask contributors to provide meaningful summaries in the opening comment, it would help everybody coming here for a review, and less experienced Git users could more easily change the text than with Git history rewrites and force-pushes.

Then, in the end, squash-merge and use the initial comment as the squash merge message. That also makes meaningless commits in the course of the PR („Fix CS“) irrelevant.

Would have saved us all some headache here, non? But that’s just my $.02, off topic here so I’ll just shut up 😉

@greg0ire
Copy link
Member

No no, you're right, I think it's just we didn't think of that! @asuri0n , can you please edit the body of the pull request, as suggested?

@asuri0n
Copy link
Contributor Author

asuri0n commented Mar 23, 2023

It's a pretty simple pull request, I've updated the body, is it enough ?

@greg0ire
Copy link
Member

greg0ire commented Mar 23, 2023

to suspend the deprecation info msg

It would be nice to quote the message. Also, it would be nice to know why you fixed reset() but not getData(). Is there no message for that one?

I think it's a deprecation coming from Symfony, right? And I think maybe you considered using @{inheritDoc}, but that is not enough for some reason I don't remember?

@derrabus
Copy link
Member

And I think maybe you considered using @{inheritDoc}, but that is not enough for some reason I don't remember?

#491 (comment)

@greg0ire
Copy link
Member

Oh right 🤦

@derrabus derrabus changed the title deprecation : add type declaration to MigrationCollector::reset() Add type declaration to MigrationCollector::reset() Mar 23, 2023
@derrabus derrabus added this to the 3.2.3 milestone Mar 23, 2023
@derrabus derrabus added the Bug label Mar 23, 2023
@derrabus derrabus merged commit 197dcc3 into doctrine:3.2.x Mar 23, 2023
@derrabus
Copy link
Member

Thank you @asuri0n!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants