-
Notifications
You must be signed in to change notification settings - Fork 631
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
Make provision to skip fields when generating Views #1166
Conversation
There is some issue in integration. I read the log but didn't understand the exact reason for the failure. Kindly help. |
@amsharma9 I'll try to sort out with this asap. |
@amsharma9 I took a look your PR and issue. Why did you apply this only to scaffold? If we add the option to config, users expect this behavior across the project. I would expected this in the model, for example. To add option in config, that excluded db fields from generated files, only for scaffold isn't obvious and must be expanded for the entire project. About code: Please, separate your code in function with one action and call it where you need if, it possible. Could you cover your code with a small console test, please? |
@sergeysviridenko I think we should get it to work with Scaffold and then look at where else we can add it. Developers might only want the fields to be skipped in Views as the data still might need to be updated in those fields via models. That is what I am doing in my project. I thought a lot about how to create a function to do this but looks non-intuitive to me. The lines for skipping have to be added where loops are running and generating code. If I create a function, then even if we rewrite the whole code we can't achieve this. There is only one place at which code is being generated and we need to write the skipping code exactly at that place. Looks more or less impossible to separate out the skipping functionality. |
I have done testing of the code. What would you like me to show you. Kindly LMK how do I pass on the test and its result to you. |
@amsharma9 About testing - I believe, it works, but we write tests for the future too. Code changes all time and all features must testing all time. So we try to cover code by tests. |
Ok, I understand your point of view. I suggest we create two options |
@amsharma9 yes you're right, this is good way. First at all this option must be added to all places where possible. Repeated code should be separated to method. And all changes should be covered by tests. Finally, make PR to 3.3.x branch, please. |
@sergeysviridenko I checked the code of Model generation, it already has an --excludefields=l |
@amsharma9 ok thank you. I'll take a look asap. |
|
Changes as specified in Issue #1165.
Kindly review if this code adheres to your standards and that it is correct and efficient.
Thanks