search

GatorCogitate / cogitate_tool

Tool to analyze contributions when working in a team.
5 stars 13 forks source link

Remove duplicate main function and its running steps #83

Closed noorbuchi closed 4 years ago

noorbuchi commented 4 years ago

This PR is intended to solve the multiple main functions issue #78 I removed the main function from data_collection.py along with the steps to run that module from the README file. Please let me know if any other changes are necessary before merging it.

bagashvilit commented 4 years ago

@noorbuchi These changes look good, thank you for your effort. We still have to make a little change. As you remember the customer expressed his concern about metric TOTAL and we decided to remove it. While I acknowledge that the purpose of this pull request is to remove the main method, because of the time limit and since the PR is already open why don't we fix that too, I think it should be a quick fix. It's also a valid argument If you think that separate PR should be opened for that.

noorbuchi commented 4 years ago

@bagashvilit I think you make a good point. I'm currently working on adding an additional test case but then I can remove the Total metric from the dictionary and processing functions.

bagashvilit commented 4 years ago

@noorbuchi It should be a quick fix, but please let me know if there is anything I can help with

noorbuchi commented 4 years ago

@bagashvilit I just made the changes you requested. I also added a test case that increases the code coverage for the data processing file. Please let me know if other changes are needed. Otherwise, please approve the PR after the build checks pass. Thank you!

noorbuchi commented 4 years ago

@MaddyKapfhammer @bagashvilit Since this is a quick fix to our current code. Should I merge it now or wait for additional approvals, I've requested several people to review but no one commented here. Please let me know what would be best to do.

MaddyKapfhammer commented 4 years ago

@noorbuchi I think that this PR looks great, and also needs to be merged to master as soon as possible. I think that you can go ahead and merge this, especially because we do not have much time left to complete the project.

noorbuchi commented 4 years ago

@MaddyKapfhammer I agree. I will wait for a little bit just in case someone adds their approval, then I'll merge it.