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

TD with query() that exposes builder #38

Closed
wants to merge 4 commits into from
Closed

TD with query() that exposes builder #38

wants to merge 4 commits into from

Conversation

bilogic
Copy link
Contributor

@bilogic bilogic commented Apr 17, 2021

Hi,

Referencing #36, I added sort as well. Let me know if any other changes required. Thank you.

@tabuna
Copy link
Member

tabuna commented Apr 18, 2021

Hi @bilogic. Please note that tests fail

@bilogic
Copy link
Contributor Author

bilogic commented Apr 18, 2021

ohh.. ok thanks, let me check why.

@tabuna
Copy link
Member

tabuna commented Apr 22, 2021

Can I help you with something so that we can integrate this?

@bilogic
Copy link
Contributor Author

bilogic commented Apr 22, 2021

Yes of course, sorry I have been busy.

@bilogic
Copy link
Contributor Author

bilogic commented Apr 26, 2021

Ok, I believe I got it fixed.

Just some feedback below:

  1. Code style: any tool you use? I'm looking at php-cs-fixer and aligned with Laravel's code style, otherwise contributors have to manually discard a lot unnecessary formatting before commiting. Maybe provide some instructions?
  2. Testing: I'm really quite new to testing, but wasn't able to set crud's up properly, I found that I don't have post table etc. In the end, I went through the logs on GitHub to pinpoint what the issue was. Maybe provide some instructions too?

Thank you!

@bilogic
Copy link
Contributor Author

bilogic commented May 4, 2021

@tabuna anything else i need to do? Thanks.

@tabuna
Copy link
Member

tabuna commented May 5, 2021

@bilogic Hi. I have local holidays now. I will be back in a couple of days and combine this

@bilogic
Copy link
Contributor Author

bilogic commented May 5, 2021

@tabuna got it! enjoy! Thanks!

@bilogic
Copy link
Contributor Author

bilogic commented Jun 1, 2021

Hi,

Does this need more changes?

@bilogic
Copy link
Contributor Author

bilogic commented Jul 5, 2021

@tabuna can we merge this?

@bilogic
Copy link
Contributor Author

bilogic commented Jul 29, 2021

@tabuna sorry, can we also make a decision on this? otherwise I have quite a bit of code hanging in the air. Thank you.

@tabuna
Copy link
Member

tabuna commented Jul 31, 2021

Hi, I would like the code that affects the builder to be in the TD class, but I don't have enough time to change it myself. It would be great if you take it on yourself.

@bilogic
Copy link
Contributor Author

bilogic commented Aug 1, 2021

Ok, I think I get what you mean, but if I'm going to move the logic into TD class, would it be better to move into https://github.com/orchidsoftware/platform's TD?

@tabuna
Copy link
Member

tabuna commented Aug 1, 2021

According to the idea, there should be a hook/event that we will define in TD (from crud)

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

Successfully merging this pull request may close these issues.

2 participants