Code Review by lyhuang@bu.edu

[huangluyang001]

Code Review by lyhuang@bu.edu
Congratulation on your finishing minipj1. However, you still have some problems. I can not run your code through your readme. Your code can ONLY be used on LINUX. On windows, when I use:
TweetPI.py annotatedvideo --timeline POTUS --limit 50 --options '{"twitter_consumer_key":"...", "twitter_consumer_secret":"...", "twitter_access_token":"...", "twitter_access_secret":"...", "google_key_json":"gapi.json"}'
I put everything in and nothing happens when I run the command(Even no errors!!!).
Default setting is important so that I can check if your code works in least time.
Basically, your code meets the MVP of this task. It can generate video from twitter pictures automatically and provide labels for them.
Advantages:

1. Your API has many functions to use. I believe users can benefit a lot from your API.
2. You have considered many conditions including:

• No json file or keys
• Twitter API fails
• No enough arguments
• FFMPEG API fails
• Photo sizes not adapted to ffmpeg

3. Users can user many functions in your API such as setting font color, size, from users’ homeline etc, which is good for those who have many requirements.
4. Naming convention is good for reader to know what the variable is as soon as possible.

However, there is still many disadvantages that I suggest to adjust.

1. First, if someone runs on windows, the code will not raise any errors, please write an environment check or adjust your code on windows system.
2. Second, too many arguments make users confused especially when they are required arguments. I need to type secrets and keys EVERY TIME I run your code, which is not user-friendly to those who just want to use as soon as possible. I strongly recommend you read keys and secrets in a given txt file or let users adjust in the code so that users do not need to type every time.
3. Third, too many functions with fewest tutorials is not recommended. I can’t even know how to use such a great number of arguments. Please write more in your tutorial.
4. Forth, Using as a library is so confused because I don’t know even know the name of your classes and their functions, could you please provide more guide to use as a library of your code?
5. Fifth, you can not deal with too many pictures situation, it is important because users may be confused if they can not get videos in a short period.
6. Sixth, default settings are important. For a starter, it is easiest and fastest way to access your API through simplest command with fewest or none parameters. I spend many times on learning to use your API, which I think is not user-friendly.

Other Details:

1. It is not a good idea to write all functions in a file. This may make your code confusing and look messing. To separate your class into different files may be a good idea.
2. More comments should be added otherwise readers may be confused.
3. Too many commits may confuse yourself. You have committed over 60 times in all the branches. This may make yourself confused when errors are found. If you setup automatic push in IDE, I strongly recommend you set a long time period.

[phy25]

Thanks for your detailed code review! I have separated them into #18, #19, #20, #21, #22.

Another thing I would like to say: with the shell's --help you can see there are a lot of arguments which is optional (with a default in the code), so maybe I need to mention that in the README just not to scare people. The config will be an optional one later in #22.

[huangluyang001]

I think maybe you should add some detailed description such as the type of the arguments and the sample usage to specify each argument and state them clearer

[phy25]

If you have time, could you please download the v0.3 version and try again in Windows? I cannot reproduce the "no output" error in Windows 10 + Python 3.7.0, and everything worked well. But there is actually a exception handling issue making "nothing outputted" possible, which has been mitigated in v0.3.

By the way, most of your suggestions have been implemented. Thanks again!

[phy25]

Closed due to inactivity. Feel free to reopen if there is update on this.