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

Behavior change in 0.41 breaks functionality of Fey::Object::Iterator::FromSelect #3

Open
kgcgs opened this issue Aug 28, 2023 · 6 comments · May be fixed by #4
Open

Behavior change in 0.41 breaks functionality of Fey::Object::Iterator::FromSelect #3

kgcgs opened this issue Aug 28, 2023 · 6 comments · May be fixed by #4

Comments

@kgcgs
Copy link

kgcgs commented Aug 28, 2023

Looks like d8efe64 broke Fey::Object::Iterator::FromSelect. The attached patch restores expected behavior in our use case.

fey-sql-select.patch

ap added a commit to ap-contrib/Fey that referenced this issue Aug 29, 2023
@ap ap linked a pull request Aug 29, 2023 that will close this issue
@ap
Copy link
Contributor

ap commented Aug 29, 2023

Fey still has users? If I was still one of them I might undertake fixing Fey::Object::Iterator::FromSelect, but as it is, reverting the patch entirely is probably a good idea. Unfortunately due to 4b07297 it can’t be done automatically, but the merge conflicts are trivial. I’ve done that and left a patch in #4 so anyone who needs it doesn’t have to do it themselves.

@kgcgs
Copy link
Author

kgcgs commented Aug 29, 2023

Well, there's at least two enterprise applications built on it that we still have running. I'm just making an effort at being a good open source user and contributing fixes and bug reports. ;)

@autarch
Copy link
Member

autarch commented Sep 2, 2023

Thanks for the bug report. I'm not using this myself either and haven't done so for ages. I'd guess that simply applying that patch would break some tests, so I'd be a little wary of applying it as-is. And of course since this code is so ancient I haven't updated it to run on a modern CI system, so even if you submitted a PR it wouldn't run any tests.

That said, maybe you could try this locally?

@ap
Copy link
Contributor

ap commented Sep 2, 2023

You marked it HANDOFF anyway. Maybe @kgcgs would be up for taking it over, as an active user, rather than having to go through I-don’t-really-care-anymore ex-maintainers/contributors?

@autarch
Copy link
Member

autarch commented Sep 2, 2023

You marked it HANDOFF anyway. Maybe @kgcgs would be up for taking it over, as an active user, rather than having to go through I-don’t-really-care-anymore ex-maintainers/contributors?

Yes, that would be great!

@kgcgs
Copy link
Author

kgcgs commented Sep 2, 2023

The patch definitely breaks the tests, if I can convince the dev that brought Fey in to spend the time to figure that out we'd be happy to take ownership of the modules.

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

Successfully merging a pull request may close this issue.

3 participants