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 ecto 3.5 comaptibilty #8

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

sztosz
Copy link

@sztosz sztosz commented Oct 19, 2020

Adds required callbacks implementations embed_as/1 and equal?/2 for Ecto.Type behaviour

@markusfeyh
Copy link
Contributor

Thanks @sztosz. Could you add unit tests that document this functionality?

@sztosz
Copy link
Author

sztosz commented Oct 19, 2020

Sure, will add them tomorrow.

@sztosz
Copy link
Author

sztosz commented Oct 20, 2020

@markusfeyh I added unit tests, and they work with Ecto 3.5, but they are not backward compatible with earlier versions because embeded_dump and embeded_load were added only in Ecto 3.4.1.

Should I just drop the tests with embed_as, which de facto tests not the Ecto.ULID library but if Ecto does it job correctly, or rather try to rewrite those tests to be backward compatible?

@markusfeyh
Copy link
Contributor

Would it be possible to add a check if the ecto version is greater than 3.5 so those tests are only run in that case?

@sztosz sztosz marked this pull request as draft October 20, 2020 18:07
@sztosz
Copy link
Author

sztosz commented Oct 20, 2020

Probably the best solution. But I also am not sure any more If it should be :dump or :self. I need to rethink it and rewrite it. But running those tests only on 3.5 makes a lot of sense.

@xward
Copy link
Contributor

xward commented Oct 26, 2020

What about doing like I did in my PR ?
Code will be managed in Ecto.Type here

@sztosz
Copy link
Author

sztosz commented Oct 27, 2020

I've got other things on my mind right now, so changed it to draft. I may find some time later in the week. Changing behaviour to just use may work, but it does not enforce that public API stays same in proper way.

@jayjun
Copy link
Contributor

jayjun commented Nov 19, 2020

Agree with @xward, I sent the same fix #5 over a year ago. 🙂

@dcuddeback If you are no longer interested in this project, may I take over?

I rely on this package heavily and evidently many do.

@sztosz
Copy link
Author

sztosz commented Nov 20, 2020

@jayjun If you fork it, update, and fix it, I'll be glad to make the project I work on switch to it. We're doing daily a lot of builds in our pipelines, so it may give new library some stats in hex.pm that would make deprecation of this package easier, that's of course with assumption that @dcuddeback will not respond in timely manner.

I totally understand that @dcuddeback does not maintain it any more for whatever reasons, and I'm OK with that. I can only thank him, and thank any person that will carry on with making ULID's compatible with current and further versions of ecto.

@jayjun jayjun mentioned this pull request Nov 20, 2020
@jayjun
Copy link
Contributor

jayjun commented Nov 21, 2020

@sztosz I managed to contact dcuddeback and long story short, TheRealReal has assigned people to look into this. They seem like an active company so I won’t consider forking yet.

@lpgauth
Copy link

lpgauth commented Feb 21, 2022

Any updates on this? Is there a better fork?

@woylie
Copy link

woylie commented Mar 19, 2022

@lpgauth Version 0.3.0 adds the required callback implementations for Ecto ~> 3.2. They just forgot to update the changelog. It was published shortly after I published a fork on Hex, which includes a few more minor changes, including the type from #6 and improved documentation.

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.

6 participants