search

esimov / pigo

Fast face detection, pupil/eyes localization and facial landmark points detection library in pure Go.
MIT License
4.37k stars 309 forks source link

define flags in main function #18

Closed davidkuridza closed 5 years ago

davidkuridza commented 5 years ago

Flags should be defined and parsed in the main function to support code modularity, simplify dependencies and allow writting independent tests.

esimov commented 5 years ago

Why do you consider that flags should be declared in the main function, since this is only a command line utility, all the logic being done in the core package.

davidkuridza commented 5 years ago

By moving flags to main function, code becomes modular meaning it's easier for everyone to see what the dependencies are, it's easier to test etc. Take a look at https://peter.bourgon.org/go-best-practices-2016/#configuration and https://thoughtbot.com/blog/where-to-define-command-line-flags-in-go for details.

I've also noticed core package in actually called pigo in a core directory. To use it correctly, it should be imported with an alias

import pigo "github.com/esimov/pigo/core"

What is your take on this? We should probably move the discussion to an issue though.

esimov commented 5 years ago

I agree. In this particular case, since is a CLI tool does not really have an obvious benefit, but taking into consideration the modularity of the package and code attribution segregation it does matter. So i will approve the merge. It has however a small downside, that you have to include each argument in the method argument list, which somehow makes it less readable.