would you be interested in a code review/refactoring?

dpastoor commented 2 years ago

Hi John,

Cool to see this CLI - as a fellow gopher + R aficionado thats a bit further along in the go journey, and would also love to see more of the combo of the two (I think go complements R extremely well), this seemed like a great repo in the public sphere to potentially refactor to be a bit more idiomatic for others to reference. That said, I can't fault "if it ain't broke don't mess", so not sure if that would be of interest to you. No harm either way, but didn't want to start messing with anything if you didn't have the time/interest/inclination.

Cheers

JohnCoene commented 2 years ago

The joy that courses through me when I read "gopher + R aficionado."

I think there is enormous potential for both languages to work together, Go is easier to write, and safe than C or C++. I'm exploring the idea of a proper integration à la rcpp but my lack of knowledge of C makes it impossibly difficult.

With regard to making it more idiomatic, I think it could be done but I'm not sure how, happy to hear of any pointers you might have.

dpastoor commented 2 years ago

fantastic - I'm traveling tomorrow but will get into this soon. I'll throw over some commits/PRs that can incrementally track some changes.

Off the top of my head we'd want to:

shift all the logic meant for the cli under internal
set up goreleaser: https://goreleaser.com/
minor tweaks to error handling missing in some places (like json unmarshaling)

example - great here: https://github.com/devOpifex/cranlogs/blob/master/data/top.go#L43 , missing here: https://github.com/devOpifex/cranlogs/blob/master/data/trending.go#L35

fix where if it hits a non-nil error could panic (at least from me scanning the code I think)

for example: https://github.com/devOpifex/cranlogs/blob/master/data/daily.go#L38 pretty sure you'd actually just get

panic: runtime error: index out of range [0] with length 0

instead could just return Daily{} in those situations

maybe add some slightly richer types so can better handle data validation for the periods etc

dpastoor commented 2 years ago

got this going here: https://github.com/devOpifex/cranlogs/pull/2

JohnCoene commented 2 years ago

Thanks Devin, this much improved the code!

dpastoor commented 2 years ago

cool deal - I'll hit up the next round of updates now

devOpifex / cranlogs

would you be interested in a code review/refactoring? #1