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

ORM service is optional #180

Merged
merged 2 commits into from
Aug 26, 2024
Merged

Conversation

mustanggb
Copy link
Contributor

@mustanggb mustanggb commented Jun 27, 2024

When using pair history ORM is now optional not required.

Removing this should allow MongoDB users to use pair history without having the ORM package installed.

@johanwilfer
Copy link
Collaborator

Thanks for the patch @mustanggb !

Did you also test this using Doctrine ORM and that still works?

@mustanggb
Copy link
Contributor Author

I'd hold off on this for now, I've not had a chance to look into the failures etc.

If anyone else wants to investigate please do.

@johanwilfer
Copy link
Collaborator

@mustanggb I see the tests about MongoDB also fails in master, as this contribution came from you could you have a look at this?

@mustanggb
Copy link
Contributor Author

mustanggb commented Aug 24, 2024

@johanwilfer
Looks like it's a bug introduced by mongodb-odm 2.8.0 (doctrine/mongodb-odm#2630), I believe it should be fixed in doctrine/mongodb-odm#2671.

Do you want to wait for the fix, or we can temporarily workaround by requiring <2.8 (or similar).

@johanwilfer
Copy link
Collaborator

I think the temp workaround sounds good, that way we get tests in master to not fail again.

If you want/can do it in this branch it should stop to fail and then we can merge this if that sounds good?

@mustanggb
Copy link
Contributor Author

mustanggb commented Aug 25, 2024

Done, unless you want them squashing into one commit.

I know GitLab supports that on merge/pull, not sure if GitHub does.

Alternatively keeping them separate would also for easy revert when the workaround is no longer needed, I'll leave it up to you.

@johanwilfer
Copy link
Collaborator

Looks great!

And yeah, Github supports squashing of merged branches into one commit (this is how it is setup in the repro currently):
image

On a personal note I perfer individual commits in the feature branch to follow the process, and to squash on merge (to follow the PR).

@johanwilfer johanwilfer merged commit 37394d8 into TheBigBrainsCompany:master Aug 26, 2024
7 checks passed
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.

2 participants