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

Re: Add support for using nested DTOs #11540

Merged
merged 2 commits into from
Aug 19, 2024
Merged

Conversation

eltharin
Copy link
Contributor

@eltharin eltharin commented Jul 3, 2024

Fork for errors correction of #11238

@norkunas
Copy link
Contributor

norkunas commented Jul 3, 2024

Your commit lost all the other authors credits 🙄

@eltharin
Copy link
Contributor Author

eltharin commented Jul 3, 2024

Take my changes and make it to yours, you don't touch your PR since 5 month but it's a super idea.

And if you can explain me how I had to do, i'm not very comfortable with PR workflow.

@norkunas
Copy link
Contributor

norkunas commented Jul 3, 2024

It's not my commits also, I've just cherry-picked to adapt for current codebase.

you don't touch your PR since 5 month

Because I've waited to get some directions what I should do? As I see you just pushed that error to the baseline

@norkunas
Copy link
Contributor

norkunas commented Jul 3, 2024

And if you can explain me how I had to do, i'm not very comfortable with PR workflow.

git cherry-pick commit-hash it's a one line and then conflicts resolving if needed :)

@eltharin
Copy link
Contributor Author

eltharin commented Jul 3, 2024

@norkunas you want to make corrections to your repo or I continue here ?

@norkunas
Copy link
Contributor

norkunas commented Jul 3, 2024

No prob if you'll continue

@greg0ire
Copy link
Member

greg0ire commented Jul 3, 2024

Please improve your commit message according to the contributing guide.

@eltharin
Copy link
Contributor Author

eltharin commented Jul 3, 2024

and if you have 10 minutes, don't hesitate to explain to me what could have been done better by email.
For doing better last time.

@eltharin eltharin requested a review from greg0ire July 5, 2024 08:37
@greg0ire
Copy link
Member

greg0ire commented Jul 5, 2024

I wrote the contributing guide (at least the part about the commit messages). Is there a part that is unclear? Also, I'd rather talk on doctrine.slack.com if you don't mind.

@eltharin
Copy link
Contributor Author

eltharin commented Jul 5, 2024

sorry I read a little so fast, I think it was "just" beacause my commit was only "corrections" without any more.

@greg0ire
Copy link
Member

greg0ire commented Jul 5, 2024

I don't think people reading the history once this is merged need to know there was a need to update the baseline. Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/3.2.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/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

@eltharin eltharin force-pushed the nestedDto branch 3 times, most recently from c8e7502 to 6a3a850 Compare July 6, 2024 11:45
@greg0ire
Copy link
Member

The docs need to be adjusted, as they say "Note that you can only pass scalar expressions to the constructor." DTOs are not scalar expressions.

This feature allow use of nested new operators


Co-authored-by: Tomas Norkūnas <[email protected]>
Co-authored-by: Sergey Protko <[email protected]>
Co-authored-by: Łukasz Zakrzewski <[email protected]>
greg0ire
greg0ire previously approved these changes Aug 10, 2024
@greg0ire greg0ire merged commit 3c95ff8 into doctrine:3.2.x Aug 19, 2024
65 checks passed
@greg0ire
Copy link
Member

Thanks @eltharin !

@greg0ire
Copy link
Member

Argh! Wrong target branch 😓
This should have targeted 3.3.x

@derrabus
Copy link
Member

Shall I do a hard reset or do you prefer a revert? 🫣

@greg0ire
Copy link
Member

Did the hard reset already

@greg0ire
Copy link
Member

@eltharin please recreate this PR, but this time, target 3.3.x please. More help on this at #11208

@eltharin
Copy link
Contributor Author

ok

@eltharin eltharin deleted the nestedDto branch August 20, 2024 12:08
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.

5 participants