Skip to content

Cobra refactor with tests#6

Merged
fantasticrabbit merged 110 commits into
mainfrom
cobra-refactor-with-tests
Jan 20, 2022
Merged

Cobra refactor with tests#6
fantasticrabbit merged 110 commits into
mainfrom
cobra-refactor-with-tests

Conversation

@fantasticrabbit

Copy link
Copy Markdown

Replacing the initial cobra refactor PR that was accidentally merged.

Refactor to implement cobra/viper and abstracted http requests for setting up tests.

fantasticrabbit and others added 30 commits October 25, 2021 12:11

@Andrew-Kulpa Andrew-Kulpa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@artur-sak13 artur-sak13 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few areas that can be improved (mostly Go "gotchas") but overall I really like where this is going. This is really impressive work especially considering that it's one of the first things you've ever written in Go. Excellent job!

By the way, are you planning on keeping the libraries under your "fantasticrabbit" namespace or do you plan on moving it under @twopt's?

Comment thread .goosarch Outdated
Comment thread README.md
Comment thread cmd/root.go Outdated
Comment thread cmd/root.go Outdated
Comment thread cmd/root.go Outdated
Comment thread cmd/getlists.go Outdated
Comment thread cmd/get.go Outdated
Comment thread cmd/getlist.go Outdated
Comment thread cmd/getlists_folderless.go Outdated
Comment thread cmd/set.go
@@ -0,0 +1,48 @@
package cmd

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I don't think you need to include a set command at this point. The config file won't be modified frequently (and probably shouldn't) and you can always override with inline environment variables (e.g. $ CLICKUP_TEAM_ID=example CLICKUP_TOKEN=pk_xxxxx clickup get task 9hz).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's a better way of generally providing Team ID. In certain use cases Team ID may not always be the same (think members of different teams using separate Clickup Spaces, but accessing from the same environment) so a machine environment variable may not work.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not suggesting a machine environment variable but a user-provided one. I have to do this all the time when using the aws-cli (or any related tooling), to get information from/deploy to our various AWS accounts. Each team member should have their own config and credentials even if they are on the same environment. One person swapping/overriding their Team ID shouldn't affect anyone else.

Comment thread cmd/set.go Outdated
Comment thread internal/auth.go Outdated
Comment thread internal/auth.go Outdated
Comment thread api/requester.go Outdated
Comment thread cmd/getlists.go Outdated
Comment thread cmd/getlists_folderless.go Outdated
Comment thread cmd/gettask.go Outdated
Comment thread cmd/getlist.go Outdated
Comment thread cmd/set.go Outdated
Comment thread cmd/set.go Outdated

@artur-sak13 artur-sak13 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! LGTM!

@fantasticrabbit fantasticrabbit merged commit f48fcad into main Jan 20, 2022
@Andrew-Kulpa Andrew-Kulpa deleted the cobra-refactor-with-tests branch July 26, 2023 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants