TylerBrock / saw

Fast, multi-purpose tool for AWS CloudWatch Logs
MIT License
1.4k stars 77 forks source link

Code/project quality #4

Closed perriea closed 6 years ago

perriea commented 6 years ago

Hi @TylerBrock

I went around the project and I realized that it was missing some stuff :

A personal opinion: I find dep very poor. Another thing the code dependencies is not versioned, which in case of deletion of a tag or a repo can break the project ... As a replacement for dep, I will use glide while waiting vgo.

I'm waiting for your feedback and your opinion :)

TylerBrock commented 6 years ago

Hey @perriea thanks for your continued interest in the saw project! I'm admittedly not a golang expert so I really appreciate the feedback and have recently made some changes based on this issue.

Overall, I believe the code quality to be reasonably good (go report card reported an A and we want to get to A+). I hope you will agree so I'd like to explain some choices I've made and work to fix some of the issues you've raised.

Golint + Misspell + Comments

I use go-plus in Atom and go.vim in vim to fix most linter errors religiously. However, there are some warnings that I've been punting on so I'm glad you are holding me accountable 😄!

As a result of this issue I've fixed the glaring misspelling of "continuously" in watch, added some comments on exported vars, and converted some vars that were unnecessarily exported to keep them private. I'd like to refactor the configuration package a bit so I'm going to hold off on documenting that bit for now if that is OK with you.

You mention that I should add additional commentary above what the linter requires in your checklist. Anything I should document in particular that you were curious about?

Missing Makefile

Running go build seems to work for me at the moment. I realize a makefile would be helpful for a larger project but do you think that is necessary (or more idiomatic) for a project of this size? If so, or if I am doing something wrong/counter culture, I'd love to fix it and do the right things. Let me know!

Dependency management

Interesting... at the time I was choosing a dependency manager for this project dep was preferred to glide and even mentioned as it's successor in the project's README. I am definitely looking to switch to vgo when available but dep seems to work for the time being.

Unless I'm mistaken, my understanding is that dep is versioning the dependencies via Gopkg.toml (directly) and via Gopkg.lock (indirectly). What am I missing here? Is it worth making a change to glide at this point?

TylerBrock commented 6 years ago

Closing this out, if you feel I haven't addressed any of the issues you brought up feel free to re-open. Thanks!