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

Merging two ~identical lists on event types #9160

Merged
merged 2 commits into from Nov 6, 2021

Conversation

ThomasLandauer
Copy link
Contributor

Just noticed that what I added in #9128 was (in other words) already there ;-)

@ThomasLandauer
Copy link
Contributor Author

Further plan:
I'd like to remove https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/reference/events.html#lifecycle-callbacks-event-argument, by integrating the passed argument PreUpdateEventArgs $event into the code sample just above it (the three ones I merged in #9159)

  1. The passed argument PreUpdateEventArgs $event is the same, no matter if you're using a listener class or a lifecycle callback? => If yes, this is a perfect candidate for another column in the overview table!
  2. Is there some reference (full list) somewhere for those EventArgs (like PreUpdateEventArgs)? If no, I'd just add a link to the very class (source code), so people can see which methods they're offering.

docs/en/reference/events.rst Outdated Show resolved Hide resolved
SenseException
SenseException previously approved these changes Nov 4, 2021
@SenseException
Copy link
Member

Can you please do a rebase?

@ThomasLandauer
Copy link
Contributor Author

Sorry, I don't know much about git :-( Rebase to 2.11.x? I've done the changes with GitHub's website - how can I rebase here? If not at all, I'd just start from scratch (i.e. copy-paste my changes), but it would be in another PR?!

@greg0ire
Copy link
Member

greg0ire commented Nov 4, 2021

Here is an adaptation of my saved reply, to help you :)

  1. Fetch all new commits: git fetch --all. ⬇️
  2. Rebase on what you fetched interactively: ✂️
    1. git rebase -i origin/2.11.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin repository.
    2. A window will show up with many lines, remove every line but the 2 last ones, which should correspond to your commit.
    3. If you run into a conflict: 💥
      1. fix it with git mergetool. 😷
      2. continue on your merry way with git rebase --continue. ⏩
  3. Force push to overwrite all this : git push --force. ⬆️

@ThomasLandauer
Copy link
Contributor Author

Thanks, but that's not working yet (see below). General question first: For the future, would you recommend doing such docs PR's via git CLI? Or (as I'm doing now) via GitHub's website?

What I did:

$ git clone git@github.com:ThomasLandauer/orm.git
$ git branch
* 2.8.x

I think there's one step missing (before your git fetch --all), namely to switch to the right branch patch-11.

@greg0ire
Copy link
Member

greg0ire commented Nov 4, 2021

Ah I was assuming you were using the CLI, and already on the right branch. I would absolutely recommend using the CLI, it will come in handy if you contribute often like you do.

If you want to start over, here is what I think you should do:

git clone git@github.com:doctrine/orm.git
git remote add fork git@github.com:ThomasLandauer/orm.git
# at that point you have 2 remotes : origin and fork
git fetch --all # this allows you to fetch extra objects (branches, commits, tags) that are only present on fork (including patch-11)
git switch patch-11

ThomasLandauer and others added 2 commits November 4, 2021 23:36
Just noticed that what I added in doctrine#9128 was (in other words) already there ;-)
Co-authored-by: Claudio Zizza <859964+SenseException@users.noreply.github.com>
@ThomasLandauer
Copy link
Contributor Author

I would absolutely recommend using the CLI, it will come in handy if you contribute often like you do.

OK, I'll try to improve :-)

I now did everything that you said at #9160 (comment), and then:

git rebase -i origin/2.11.x
# In nano, deleted everything except:
> pick 228365466 Merging two ~identical lists on event types
> pick 20b868fb7 Update docs/en/reference/events.rst
git push --force

Now I see "This branch has conflicts that must be resolved" on the website (didn't see it in the CLI). Should I click on "Resolve conflicts" on the website? Or CLI?

@greg0ire greg0ire changed the base branch from 2.10.x to 2.11.x November 4, 2021 23:02
@greg0ire
Copy link
Member

greg0ire commented Nov 4, 2021

You don't have anything to do on your computer now. I just clicked "edit", changed the target branch to the correct one and the conflicts are gone. Well, they are still here, but will get fixed by the poor soul that will have to merge 2.10.x into 2.11.x

@greg0ire greg0ire closed this Nov 4, 2021
@greg0ire greg0ire reopened this Nov 4, 2021
@greg0ire
Copy link
Member

greg0ire commented Nov 4, 2021

Closing and reopening to trigger the CI jobs.

@ThomasLandauer
Copy link
Contributor Author

would absolutely recommend using the CLI, it will come in handy if you contribute often like you do.

I've thought about this some more. But I will continue doing Docs PR's on GitHub's website. Why:

The main reason is that my "workflow" differs from (what I guess is) yours: Whenever I (accidentally) see something in the docs that could use some improvement, I click the "Edit" button, type my suggestion, and I'm done. I just don't need a clone of the entire project on my machine: no need to run run tests; usually just one file affected; no need to actually see if "it works". So those 5-10 CLI commands would just add overhead for me, and in fact worsen my DX (starting by having to search the very MD/RST file myself; no preview in my IDE; back-and-forth between CLI, IDE, and GitHub...)

But thanks for your instructions! I'm saving them for problem cases and larger undertakings.

If you don't want your PR queue getting clogged with some minor PR typo fixes :-) then I think the only way to prevent this is by separating the docs into a dedicated repo, like e.g. Symfony did it.

@SenseException SenseException merged commit 1e971d8 into doctrine:2.11.x Nov 6, 2021
@SenseException
Copy link
Member

Thanks, you two :-)

@ThomasLandauer ThomasLandauer deleted the patch-11 branch November 6, 2021 20:34
ThomasLandauer added a commit to ThomasLandauer/orm that referenced this pull request Nov 10, 2021
As announced in doctrine#9160 (comment) I'm adding the passed "EventArgs" class to the overview table. Once this is complete, my further plan is to remove the entire paragraph https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/reference/events.html#lifecycle-callbacks-event-argument, and probably also the second code block at https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/reference/events.html#entity-listeners-class

Is there a better way to link to the source code of `LifecycleEventArgs` than https://github.com/doctrine/persistence/blob/2.2.x/lib/Doctrine/Persistence/Event/LifecycleEventArgs.php ?

Also, I changed `postLoad` to `preUpdate` in the code block, to have an example that does not receive `LifecycleEventArgs` ;-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants