This repository was archived by the owner on Mar 27, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 237
Single image analysis #20
Merged
cftorres
merged 8 commits into
GoogleContainerTools:master
from
cftorres:SingleImageAnalysis
Aug 18, 2017
Merged
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ad78c06
Adding single image analysis
cftorres 6b359d2
Merge remote-tracking branch 'upstream/master' into SingleImageAnalysis
cftorres 414dd3a
Addressing Abby's comments
cftorres 9d4ef87
Addressing Nick's comments
cftorres b453dd4
Updated integration tests
cftorres 2d09bc5
Modified README
cftorres f38e8a5
Tweaked README after Abby's comment
cftorres 6c212b1
Tweaked README after Nick's comments
cftorres File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes more sense to me to have analyze as a separate command (such that the logic wouldn't be in root.go) both for clarity and for example in case of future additional commands. If someone adds a command and pattern matches, they seem forced into expanding this if else structure. The primary logic already seems separated out so is there a reason not to pull it into it's own command file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought only one command would be preferable for our tool... @nkubala @aaron-prindle do you have any thoughts as to whether we should split this up into a separate command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think this structure is fine. We should probably at least put a comment here saying semantically what each case means (
analyze
= one image,diff
= two), and then put something about it in the usage/--help
command. I do like the tool choosing which path to take based on how many images you provide it though, seems intuitive to me