search

GatorCogitate / cogitate_tool

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

Merge Command line interface into Master #89

Closed JMilamber closed 4 years ago

JMilamber commented 4 years ago

The Command Line interface is now passing Travis and has added all functions mentioned in issue #64.

Please let us know if you see anything that needs changed or explained.

codecov-io commented 4 years ago

Codecov Report

Merging #89 into master will not change coverage by %. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #89   +/-   ##
=======================================
  Coverage   70.15%   70.15%           
=======================================
  Files           4        4           
  Lines         392      392           
  Branches       68       68           
=======================================
  Hits          275      275           
  Misses        109      109           
  Partials        8        8
noorbuchi commented 4 years ago

After implementing the progress bar, one of the test cases involving capsys is failing, I'm not sure how to fix that test case but I wanted to let you all know about why the build is failing.

noorbuchi commented 4 years ago

@bagashvilit I think that this is a good idea, but I feel like it would work better for the web interface since we are not very worried about the graphical representation of the data in the terminal. What do you think?

JMilamber commented 4 years ago

@noorbuchi Ill fix the test case when I work on this later. I might have to add an argument that disables the loading bar while testing. We’ll see thanks for the heads up.

I agree with noor about the extra metrics, its going to add a lot of extra work to allow the user to specify them in the arguments for the cli.

JMilamber commented 4 years ago

I have another suggestion for this PR, why don't we let the user pick which metrics they want to display from the table, If I am getting it correctly, currently we are only displaying few of those metrics, there is no way for the user to display other metrics.

@bagashvilit while this would be a cool feature, adding this in currently would be very clunky, and too much, as @noorbuchi said, on the side of what the web interface is supposed to do.

JMilamber commented 4 years ago

After implementing the progress bar, one of the test cases involving capsys is failing, I'm not sure how to fix that test case but I wanted to let you all know about why the build is failing.

Looking at the test case @noorbuchi, its because the test case is reading in terminal output and checking it, which now includes the progress bar you implemented. Fixed the test case.

noorbuchi commented 4 years ago

@JMilamber I just checked the changes you made, I think they're great. There are still some changes needed to handle if the username is not a part of the dictionary and if the keep and remove usernames were identical. I've been working on adding a merge duplicates functionality to the web interface so I also think it would be nice to give the user the option to either merge in the terminal window or in the web interface. It's not completely ready in the web interface yet and there are few fixes needed but I suggest that you check it out and let me know what you think. Thanks for your work on this.

cklima616 commented 4 years ago

This pull request is ready to be assigned reviewers and hopefully get merged to master!

cklima616 commented 4 years ago

Improvements in test coverage are still necessary - I tried editing the coverage implementation doc to not include the if name... and main() methods, but I’m not sure if I did it completely/correctly.

noorbuchi commented 4 years ago

@cklima616 I think that we can start the review process, but there are still some needed changes such as the ones discussed with @JMilamber regarding the json files and the merge process

JMilamber commented 4 years ago

@JMilamber I just checked the changes you made, I think they're great. There are still some changes needed to handle if the username is not a part of the dictionary and if the keep and remove usernames were identical. I've been working on adding a merge duplicates functionality to the web interface so I also think it would be nice to give the user the option to either merge in the terminal window or in the web interface. It's not completely ready in the web interface yet and there are few fixes needed but I suggest that you check it out and let me know what you think. Thanks for your work on this.

Thanks man! working more on this in the next couple hours, ill take a look at the web interface when I get a chance!

@noorbuchi I looked at it and unfortunately realized that I don't know how to run the web interface so that I can see it. xD If you could get that to me I will look at it

I added the functionality for choosing to not overwrite the json files, let me know what you think.

Still need to edit the merge duplicate usernames and add functionality to except the same username being given for both keep and remove, gonna try and do that tomorrow.

noorbuchi commented 4 years ago

@JMilamber The command to run the web interface is streamlit run src/web_interface.py just make sure you're in the repository folder and not in the src or any other folder. I will check your latest changes and let you know. Please don't forget that we need to also account for input that does not exist, which can happen through a typing error but would cause the program to crash due to key error. You'll need to check if the username exists in the dictionary first.

cklima616 commented 4 years ago

@noorbuchi I started some of the changes - as far as where check_Json should be, I'm not sure. Let me know if there are other edits to make.

noorbuchi commented 4 years ago

@cklima616 thank you for adding them. As of right now, we're just waiting for @JMilamber to address this question since he worked on that part. Also. there are these changes that we've talked about that still need to be added to prevent the program from crashing. I mentioned these changes in one of my previous comments and I quoted it below. I think we are really close to getting this PR merged. I think that we already have it so the user can choose whether they want to go into the web interface or not so that part is dealt with.

@JMilamber I just checked the changes you made, I think they're great. There are still some changes needed to handle if the username is not a part of the dictionary and if the keep and remove usernames were identical. I've been working on adding a merge duplicates functionality to the web interface so I also think it would be nice to give the user the option to either merge in the terminal window or in the web interface. It's not completely ready in the web interface yet and there are few fixes needed but I suggest that you check it out and let me know what you think. Thanks for your work on this.

JMilamber commented 4 years ago

@noorbuchi just got back to this, sorry its been a bit, looked at where you moved the check Json to and that looks great! Is there anything else that needs done? I believe @cklima616 fixed the other part of what you quoted above, though I haven't checked it yet, going to now.

noorbuchi commented 4 years ago

@JMilamber no worries, I've just been tweaking some stuff. The only needed thing right now is to handle user input for the merge duplicate users, we should handle for all inputs that do not exist in the dictionary keys, and if both the keep and remove usernames are the same. We can just display a message and ask the user to reenter a proper input.

JMilamber commented 4 years ago

@noorbuchi @cklima616 Everything should be done now, take a look and let me know what you guys think. The next day->2 days are pretty busy for me but if there is something else ill do my best to get to it tomorrow night. Hopefully fixed the linter errors that were breaking the build. need to go to bed so fingers crossed they are fixed.

noorbuchi commented 4 years ago

@JMilamber No worries, I fixed the remaining linter issues and everything should be working fine. We'll hopefully get this reviewed and merged as soon as we can. I don't think that any other changes are needed.

JMilamber commented 4 years ago

@noorbuchi thanks for fixing the linter stuff!

noorbuchi commented 4 years ago

@JMilamber no problem, happy to help!

JMilamber commented 4 years ago

@MaddyKapfhammer @bagashvilit @noorbuchi @enpuyou I'm good to merge this right? Or should I wait for more reviews?

noorbuchi commented 4 years ago

@JMilamber I would like to see more reviews from other people but if no one reviews it soon, you can go ahead and merge this PR, unless someone prefers otherwise.

JMilamber commented 4 years ago

@noorbuchi if no one else reviews it by tomorrow night ill merge it. Sound good?

I somehow closed this on accident lol. Re-opened it.

noorbuchi commented 4 years ago

@JMilamber that's reasonable. I think we should merge it tomorrow night by the latest.