Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add cobra cli library #89

Merged
merged 5 commits into from
May 22, 2024
Merged

feat: add cobra cli library #89

merged 5 commits into from
May 22, 2024

Conversation

oversize
Copy link
Contributor

@oversize oversize commented May 21, 2024

This PR adds cobra as the cli parsing library. #63

  • I have copied the way cardano-up has used cobra more or less exactly
  • The cli currently has the same ability as before. I wanted to start some better cli then "cli" but felt that it would be better to have some more discussion before implementing it. I will add my thoughts/ideas there
  • I removed internal/logging in favour of internal/consolelog also stolen from cardano-up
  • I needed to remove some loggig output from the api, because i dont know gin and did not spend time figuring out how to adapt slog to work with it. Also: I would like to suggest to remove gin and use go's 1.22 new stdlib net/http muxer instead. Removing the dependency alltogether. The api seems fairly simple so i think net/http is all we need. But i will open. another ticket for that discusson.

@oversize oversize marked this pull request as ready for review May 21, 2024 20:08
@wolf31o2
Copy link
Member

Looks like you have conflicts with main.

@oversize
Copy link
Contributor Author

oversize commented May 22, 2024

Looks like you have conflicts with main.

Oh snap, yes. I have merged these changes now and updated the commits.
I also just saw that cobra still provided its default "generate shell completion" script, which i now have disabled.

Copy link
Member

@wolf31o2 wolf31o2 left a comment

Choose a reason for hiding this comment

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

Back out the changes to remove the internal/logging module. I'm fine with the introduction of slog for console logging, but we need the access logs in the API to stay.

cmd/bursa/new.go Outdated Show resolved Hide resolved
internal/api/api.go Outdated Show resolved Hide resolved
internal/api/api.go Outdated Show resolved Hide resolved
@oversize
Copy link
Contributor Author

Back out the changes to remove the internal/logging module. I'm fine with the introduction of slog for console logging, but we need the access logs in the API to stay.

I have removed the console logging again in favor of internal/logging. I'll look into gin and how to have access logs with log/slog so we dont lose these when switching to it.

@wolf31o2 wolf31o2 merged commit 6147eb1 into blinklabs-io:main May 22, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants