search

vanshg / MacAssistant

Google Assistant for macOS!
MIT License
1.62k stars 125 forks source link

Add images to Build Instructions #37

Closed dlh3 closed 6 years ago

dlh3 commented 6 years ago

Opening a PR for issue #35, so it is easier to review and comment on.

@navanchauhan: These minor changes are pretty fractured, with several duplicate and undo commits. Perhaps you could squash these all into a single commit?
Hint: if you're not sure how to do this, try git rebase -i 16a394293d76f35a5ea7e8b0d365b71b8eed21de~.

vanshg commented 6 years ago

This is unnecessary for the README

dlh3 commented 6 years ago

Hi @vanshg, I'm a little surprised that you closed this PR. I referred back to #35, and you agreed that the images would be a good addition. I agreed, as did at least one other user who gave the issue a thumbs up.

I'm curious, what has changed. Why not include images for the build instructions?

More context: I opened this PR because another user, @navanchauhan, proposed adding images to the build instructions (README) in #35 and had even pointed to his changes, but I felt that it could benefit from being a PR (because I wanted to recommend that @navanchauhan rebase to cleanup/combine the various commits; though they could also just be squashed while merging the PR).

vanshg commented 6 years ago

I only just reviewed the actual pictures that were added. The pictures simply don't add much value. It's one picture of a command inside terminal. And another picture of a file in Finder. I don't think it's really necessary to have pictures for these items, they're fairly self explanatory. I do appreciate your concern and the time you and @navanchauhan have put into this.

Also a more relevant reason: since this PR was made, I began revamping the project to be a little bit more reliable. So as a result, the make command for grpc isn't really necessary unless upgrading Assistant API versions. Also, the location of the oauth json file has also been changed. So really, both of these pictures are kind of out of date now and it wouldn't make sense to add them.

dlh3 commented 6 years ago

Thanks for the quick reply and for offering some more information. I agree that the images in the PR weren't the most informative.

I haven't looked at this project in several months, but I just reviewed the README. I think the build instructions could still be improved with some images, especially the part about generating OAuth credentials in the Google Developer Console. But I agree that this PR did not cover that.

vanshg commented 6 years ago

Yeah, totally agree. The instructions are also kinda incorrect and out of date now and the UI pictures are out of date. I'll try to update the entire README soon