Code Review #1

[Danc2050]

Summary

Overall, it looks like a decent start. Some pieces are starting to form as it looks like you all have divided up some of the features to work on. Connecting them and organizing your code structure so you can properly test them will be helpful steps. I apologize in advance that this is so long, but I also apologize if I missed anything (I moved from my local machine editing files, to using GitHub issues).

Specific File Code Reviews:

README

• The README should be updated. I don't encourage updating the README with every single detail, since a lot of things can change and you are wasting time + work, but I think you guys know enough at the present moment to add how to run the basic program (e.g., python3 __main__.py -S [script_name]) and also to talk about what the project is about. You may also decide to include the help menu for your __main__.py as well. If one person updates the README, all 6 other people on the team (plus us sponsors) will know how to run the program.
• It would be nice to have myself listed (Teal will let you know if he wants to be listed in the README or not). Ex:

# Sponsors
[Daniel Connelly](https://www.linkedin.com/in/dconnelly2/)

• Should be very detailed at the end of this project.

debugLogFile

• This file's class/functions are never created/called anywhere.
• There are no comments describing anything.
• It may be a feature you are adding later, but I think there is some confusion on how the config files will work as I don't actually see a config file in any part of this project. There should be a config file for various things (e.g., whitelist or blacklist, cloud program options, etc.) and it should be in your project repo (perhaps like config_files/ and cloud/config_file?) by default. In other words, there shouldn't be a function that writes the config file each time, as it should exist in perpetuity. I don't know how well your current config library works, but you may check out configparser to update .ini files if it is easier for you. I will try and verify all the config files that need to exist when you guys develop your requirements doc further.

UserScript

• I would suggest specifying this file as main.py or __main__.py.
• Fix the doc strings to align with Pythons 3.8 doc string style guide.
• This except https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/executeUserScript.py#L30, should handle the basic case of a user entering the wrong script name. We don't want to submit that as a bug to a project!
• This https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/executeUserScript.py#L31 is a python2 way of string formatting. f-strings are SIGNIFICANTLY faster and easier to read than using python2's .format() (see this f-string article for a discussion on performance). I think this should work as a substitute: print(f'{self.scriptName} did not exit gracefully, Submit a Bug!"').

executeUserScriptTests

• This test and any other test should be in a Tests/ folder. Please also note that you should have some sort of CI (e.g., Travis CI) to run these tests on each push to make sure the output is correct.
• You may also choose to use an actual testing framework.
• There are no comments/doc strings.
• This test https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/executeUserScriptTests.py#L7-L13 actually fails since there is no key for that subprocess called text in Python 3.6+. See, and reference in the future, the updated documentation. The same is true for this function https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/executeUserScriptTests.py#L15-L21.

filterLists

• There should always be a path for this https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/filterLists.py#L13 file, even if the project owners do not use it. If the path doesn't exist, the program shouldn't continue because it has nothing to filter. The same is true for lines 16 and 19 (although read the fourth-to-last bullet in this section about white lists and black lists).
• IMO, the check_in_both https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/filterLists.py#L23 function should not exist and this work should be done in the constructor. If there is an error, it should be sent to the project owner (I am assuming you can place an email in some config file to email the owner if there is a problem in their setup of your autobug software. Or potentially you could file a bug in the Issues of their repo.
• Even so, this https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/filterLists.py#L29 should be a print statement if it is a WARNING. Otherwise, say ERROR and halt the program like you do.
• This https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/filterLists.py#L37-L39 can be optimized to: self.bugs += [item for item in self.white_data if item not in self.bugs]. However, I am confused. Is the blacklist bugs you don't want to capture and the white list bugs you do want to capture? 1) You already added in any bugs that were not in the blacklist... I think this is enough and you don't need a white list? Anything else, by default, should be sent as a bug, since the repo owners don't know about it to put it on the Black List. I am sorry if I misled you when I was suggested white lists or black lists. Even if this wasn't an issue, you are adding in all bugs in the white list if they aren't in the self.bugs array (i.e., when they aren't in the bug output, you are adding them)? I think you want in self.bugs not not in.
• Using the in keyword may get you in some trouble when there is similar output that is not a bug. If anyone is good at regex, they should maybe brainstorm how to use that to find bugs in code, but at the moment its an okay start.
• I think you mean file not array here https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/filterLists.py#L43.
• This logic https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/filterLists.py#L45 is taken care of in the constructor. You should exit in the constructor if it doesn't exist, since there is no bug report to filter.
• When you get the pieces connected, you should not create a "new" report https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/filterLists.py#L41. If the process is halted, or does not work for some reason, the raw bug report will be sent, which may not be the filtered version. It is much wiser to just pass the bug report to this function and then, when it is finally filtered, you can write it out to disk and send it.
• this https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/filterLists.py#L48 puts a space between every character of the self.bugs (e.g., "T H I S ...). Maybe you wanted "".join(line) + "\n"?.

read_config

• Needs a doc string in the __init__.
• Convert this
  https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/read_config.py#L18 to an f-string for performance.

read_config_test

• Needs to be in a tests directory.
• On line 25, you have test = os.path.join(Path.home(), BUGTRACKER). The RHS of the equal sign is repeated multiple times in the code. Make it a global or put it in the testInit class and reference it from this line on when needed.
• all the code below this https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/read_config_test.py#L31-L38 is a duplicate of the code directly above it, except for the print statement (which is logically equivalent to the above one) and the os.remove, which can just be called at the end, outside both if and else functions.
• Same issue with the duplicate os.remove here https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/read_config_test.py#L53-L58. Also present on line 85,88.
• Why don't you instead make a for loop that iterates through a list of tests? This would eliminate 4+ lines as you are printing the same thing and you can also add more tests in more easily. You will need to keep another list that holds what you are expecting the result of test to be. Please also make line 53 less confusing and just say if test. Also you can say on line 62, if not test.
• You overwrite this variable https://github.com/ismustachio/TheBugTracker/blob/b97d43fe15460ec1de2c1037d0d43b228f969c33/read_config_test.py#L77 on line 79 after doing nothing with the result of it. I think this is doing something, but on the face of it, I can't tell what it does. It needs a comment.

General Issues/Comments/Suggestions

Feel free to take the feedback with a grain of salt and ask questions.

• Although you have not developed any code for this yet, you should also have a directory for the Cloud piece of your software (e.g., Cloud).
• To reiterate, you all should read the Python style guide (PEP8) since you are writing Python code. Especially read the documentation strings (AKA doc strings) section and go through all the files and fix these errors (this was the most repetitive style error).
• Doc strings are not given to every function, especially in the test files and where there are init functions. You should also have a doc string at the top of every file to indicate its main purpose.
• check_output is a fine function, but you may actually find that Popen will be more useful for your cases. You can get stderr, a return code, etc. more easily this way. See a good intro to this function here. You can actually check the STDERR specifically for potential bugs and avoid (or use less) the checking of standard output.
• I am thinking that maybe a whitelist or a blacklist should actually come from the cloud instance or some online endpoint, since developers may want to change this anytime, but may not want to release a new version just to update a white and/or black list. Something to consider moving forward.
• I believe Antonio said that the name of the project will be AutoBugTracker. The repository should be renamed to the same name.

Let me know if I can be of any help in clarifying any of the above or answering any more questions.

[tdulcet]

I second everything @Danc2050 said! Since there is not a lot for me to test yet, I am going to focus on some higher-level issues:

README

• You need to provide very detailed instructions on your README for how to setup and run this on every supported platform (Linux, Windows and macOS). See here or here for an example.
• Feel free to list my name as a sponsor:

# Sponsors
[Daniel Connelly](https://www.linkedin.com/in/dconnelly2/)
[Teal Dulcet](https://www.tealdulcet.com/)

Testing

• You have some tests in the repository. You need setup CI to automatically run those on every commit and PR. I would recommend Travis CI, which is free for open source projects. Here is a very simple .travis.yml file you can use to get you started, which will test against the three latest versions of Python:

language: python
python:
  - "3.6"
  - "3.7"
  - "3.8"
install:
  - # Install dependencies, e.g. pip install -r requirements.txt
script:
  - python3 __main__.py --help
  - # Run tests, e.g. pytest

• If you have a favorite Python linter, such as Pylint, you should run that too.
• There should be test programs written in multiple languages besides Python, both compiled and interpreted. C, C++, Java, Rust and JS (Node.js) would be a good start. Here are example Hello World programs to get you started.

General

• This repository should be transferred to a new GitHub Organization specific to this project, so that is it easy for us to access when you are done. That way you can create a separate repository for the Cloud portion.
• The repository needs a License. The MIT License is a good choice, since it allows everyone to use the project. GitHub has a tool to add it. Also, you need to add something like this to the top of every file, in addition to the doc string as Daniel explained:

 * Copyright (c) 2020 PSU CS Automatic Bug Tracker Capstone Team
 * This code is available under the "MIT License".
 * Please see the file LICENSE in this distribution for license terms.

Please let me know if you have any questions.

[Danc2050]

All looks good in @tdulcet 's comments as well. The only thing I would reemphasize is that during our voice talks (which Teal could not make) I did not require a testing framework to be chosen. That being said, it could potentially help, so that one is definitely up to you.