-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Merge branch '4.2' #5060
Merge branch '4.2' #5060
Conversation
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: Mostafa Khudair <59371810+mostafakhudair@users.noreply.github.com>
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
* Add missing make:validation command * fix shot title
7d13190
to
26244f9
Compare
Sorry Paul, I had done most of this last night and should have pushed it up - I didn't know you were going to work on it! We should compare results with this and #5061 because the rebase was messy and I probably made a few mistakes. |
This one is fairly ready. I just don't know how to fix PHPCPD. I don't know why the total commits in yours differ from mine. Maybe because empty commits during rebase got discarded? This PR is just missing 2 commits from yours (which I believe are just empty commits) |
Yes I think that's the case - there were a couple of commits that I manually adjusted to the proper styling only to find that a layer commit was already going to do that, so those probably got removed. I will check out PHP CPD when I'm on desktop but let's plan to go with this one. |
Not sure what was happening there, those lines weren't duplicates at all! Since we were already excluding part of SQLSRV I added Forge as well, not a big deal. |
Thanks, @MGatner . Maybe at some point I can take a look at the exclusions to see a proper fix for them. Thanks for the review. |
I think the marked passages are quite similar, except for the use of |
I also noticed that in a way that schema is the only difference so I was looking on how to extract the common functionality. Im not yet familiar with the options but I think I saw it can. |
Yes you can adjust the threshold with CLI parameters. If someone wants to dig into this please do but it also isn't a big deal to exclude some files. |
Agreed. However, as Paul said, the duplicate parts are extractable in principle. The general question would be, how much do we want to modularize for the sake of not duplicating code. |
Description
develop
Checklist: