[SERVER-17349] Source Code refactor - Abstract common functionality Created: 23/Feb/15 Updated: 17/Mar/15 Resolved: 09/Mar/15 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Security |
| Affects Version/s: | None |
| Fix Version/s: | 3.1.0 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Krishna Narasimhan | Assignee: | Ramon Fernandez Marina |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Backwards Compatibility: | Fully Compatible |
| Participants: |
| Comments |
| Comment by Ramon Fernandez Marina [ 09/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
I forgot to add ticket numbers in the revision history – my apologies. Here are the relevant commits:
krishna.nm86, thanks for your contributions. I'd like to ask to please follow these guidelines for future pull requests:
I'm going to resolve this ticket and its subtasks. Having a sub-task per pull request is a bit of an overkill, and if all pull requests are in one commit opening one ticket is enough Thanks again, | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 06/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
If the test worked, I could make the changes and update the pull request | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 05/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Thanks a lot. Would be an exciting result if the approach could have results mergeable by good quality repositories. | |||||||||||||||||||||||||||||||||||||||
| Comment by Ramon Fernandez Marina [ 05/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Found a simple typo in #927, where ";" and ")" need to be exchanged:
There's also some trailing whitespace which is flagged by style checkers. I'm manually fixing those two things and running the result through our testing. | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 05/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Hi, I am using an Eclipse plugin which I wrote myself that identifies potentially copy-pasted code and abstracts them. Of course, the ultimate decision is upto the maintainers to accept a pull request if it makes sense in the long run for them. These 2 particular pull requests were part of a prototype evaluation of my tool , in order to decide further working based on it with the percentage of accepted merges. Especially #928 seems to have saved a lot of redundant code space and reduced code duplication , so would be nice to hear if one orb oth of these could be deemed acceptable by your code standards. Thanks, | |||||||||||||||||||||||||||||||||||||||
| Comment by Ramon Fernandez Marina [ 05/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Hi krishna.nm86, I'm not able to compile #927 as it is, haven't tried #928 yet. Can you please provide some more details on the refactoring software you're using and the context of your work? It would be useful to know a bit more about the bigger picture: while abstracting common functionality is in principle a good thing, some of your pull requests may affect other/larger refactoring/rewriting plans so it may not be possible/convenient for us to accept them at this stage. Thanks, | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 04/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Thanks a lot. Actually this is part of the evaluation of Programming Re factoring research approach and would be really nice to get feedback or successful merges from high quality code bases like yours. Regards | |||||||||||||||||||||||||||||||||||||||
| Comment by Andreas Nilsson [ 03/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
krishna.nm86 we have been a little caught up with the 3.0 release, sorry for the delay in getting back to you. I am reassigning the ticket to ramon.fernandez who handles our external pull requests. Thank you, | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 03/Mar/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Hi @Andreas Nilsson, | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 27/Feb/15 ] | |||||||||||||||||||||||||||||||||||||||
|
I would like to merge these two pull requests | |||||||||||||||||||||||||||||||||||||||
| Comment by Andreas Nilsson [ 27/Feb/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Hi krishna.nm86 can you elaborate on what you are looking to do with these tickets. Do you have a pull request you would like to merge or are you suggesting changes/bug fixes to the server? Best regards, | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 25/Feb/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Thanks . I have made one small fix to the change I made. Hope that helps. | |||||||||||||||||||||||||||||||||||||||
| Comment by Ramon Fernandez Marina [ 24/Feb/15 ] | |||||||||||||||||||||||||||||||||||||||
|
krishna.nm86, when a ticket has the "Needs Triage" fixversion the ball is in our court to decide if a ticket will be part of the server, and if so, where does it fit in the planning. Bottom line, no action item on your end as of now. | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 24/Feb/15 ] | |||||||||||||||||||||||||||||||||||||||
|
I am unfamiliar with the terminology here. What does needs triage mean. Is there something i need to do now. Or just wait for feedback | |||||||||||||||||||||||||||||||||||||||
| Comment by Krishna Narasimhan [ 23/Feb/15 ] | |||||||||||||||||||||||||||||||||||||||
|
Hi Andrew. The rationale behind this refactor is to reduce the amount of redundant code and using Abstraction practices to keep in sync with the Software Engineering principle of "Donot Repeat Yourself (DRY) " | |||||||||||||||||||||||||||||||||||||||
| Comment by Andrew Morrow (Inactive) [ 23/Feb/15 ] | |||||||||||||||||||||||||||||||||||||||
|
This project is for the C language MongoDB driver, but the functions you identify above are part of the MongoDB server, mongod. The correct project for issues with the server is the SERVER project. Can I refile these tickets for you in that project? Additionally, you should provide some motivation or rationale for the changes that you are suggesting. Thanks, |