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

Feature request: add query bean copy() method to TQRootBean interface #3374

Closed
Incanus3 opened this issue Mar 28, 2024 · 2 comments · Fixed by #3377
Closed

Feature request: add query bean copy() method to TQRootBean interface #3374

Incanus3 opened this issue Mar 28, 2024 · 2 comments · Fixed by #3377
Assignees
Milestone

Comments

@Incanus3
Copy link
Contributor

This request builds upon my previous request where I asked for generated query beans to have copy() method, which you added and I'm very grateful for that. The addition of the method to the generated beans solved most of our problems we had with this, but there's still one thing missing - if you have a generic code that works with subclasses of TQRootBean, it can't call this method, because it's not part of TQRootBean interface. Do you think it would be possible to add public abstract R copy() method to TQRootBean and then mark the generated copy() methods as @Override?

Expected behavior

data class QueryWrapper<E : Any, Q : TQRootBean<E, Q>>(val query: Q) {
    fun page(number: Int, size: Int): QueryWrapper<E, Q> =
        copy(query = query.copy().setMaxRows(size).setFirstRow(size * (number - 1)))

    fun toList() = query.findList()
}

I'd like this to compile and work.

Actual behavior

Unresolved method query.copy()

Additional info

If you decide this makes sense for you, I can try to implement this and create a PR.

@rbygrave rbygrave linked a pull request Mar 30, 2024 that will close this issue
rbygrave added a commit that referenced this issue Mar 31, 2024
querybeans: #3374 - Add copy() method to TQRootBean
@rbygrave rbygrave self-assigned this Mar 31, 2024
@rbygrave rbygrave added this to the 14.1.0 milestone Mar 31, 2024
@rbygrave
Copy link
Member

So with the PR above we have added copy() to TQRootBean.

As a followup, TQRootBean is a pretty unfriendly name and is not an interface. It would be better for there to be an interface like QueryBean that it implements and/or a better name like BaseQueryBean / QueryBean.

I'm pondering this, my gut is leaning towards "we should really have an interface here" so:

  • Add a interface QueryBean
  • Ideally rename TQRootBean to say BaseQueryBean

Background: TQRootBean has a funky name because (A) I didn't predict this type of use of query beans (and to some extent how important and useful they would become) and because (B) design wise we have "Root Beans" [TQRootBean] and "Associated Beans" [TQAssocBean] with the "Associated Beans" existing to specifically deal with the relationship case and are effectively an implementation detail that most people don't know anything about (so its ok using the funky TQAssocBean name there imo).

I'm sitting here today after many years of using query beans and thinking that they are so very important and that points towards adding a interface QueryBean.

@Incanus3
Copy link
Contributor Author

Incanus3 commented Apr 1, 2024

Thank you so much for adding this so fast, we really appreciate it. And yeah, it hasn't bothered me till now, but now that I think about it, I'd have to agree TQRootBean is not the most intention-revealing name I've seen ;).

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

Successfully merging a pull request may close this issue.

2 participants