-
Notifications
You must be signed in to change notification settings - Fork 1
Feedback #1
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
Comments
It is important for me that this solution is correct just for now. My problem is DTO for data transfer. URF.Core can be add as a package as you mentioned. Project and Solution have the same name because this is only a sample solution in this package. Is the solution correct? |
The approach looks good with the interface and class you added. But the best way to tell is for you to add a sample project that uses your solution. Would it be too much trouble for you to refactor the project to add the URF package instead of the source code? You should also add some unit tests. Let me know if you need help with any of this. |
Hi Tony. Firstly, I changed the name of the project. And, I have added the necessary packages from URF.Core. Test.DomainModelService is a unit test project. I tested Insert, Update and some methods. |
Terrific! I'll build and run the tests. Say, can you please correct the links in your comments? See this. |
I’ll be able to give this a try sometime today. |
Created PR #2 with my refactoring. Please look at my changes, and I can answer any questions you have in the PR comments. |
Any questions abouty refactoring? Does the Update method look OK? |
When using the update method be careful. Because, If a property is missing on the DTO. This property is setting null when mapping DTO to the entity. Your solution is better than the first solution. It's clean. |
Rather than adding any code from URF.Core, what you need to do is simply add the URF.Core packages that you need.
You want to have just two projects:
Your solution should be the same name as your repo: URF.Core.DomainService.sln
Does this make sense?
The text was updated successfully, but these errors were encountered: