Refactor the command utils package #33
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- decrease amount of duplicate code - add some comments - update readme
samyfodil
requested changes
Sep 19, 2023
cli/command/argParsingHelpers.go
Outdated
|
|
||
| import "github.com/urfave/cli/v2" | ||
|
|
||
| func prependArgParsingToAction(c *cli.Command, argName string, argGetter func(ctx *cli.Context) (string, error)) cli.ActionFunc { |
There was a problem hiding this comment.
I see the point but this function makes it harder to read and understand the code. So I suggest you drop it.
There was a problem hiding this comment.
Fair. I thought there should be a helper for that but I agree that it ended up being very verbose and somewhat complicated. Removed it in 5d616c3
cli/command/universeN.go
Outdated
|
|
||
| // Attaches universe argument parsing to a command. | ||
| // `pos` is position of the universe argument, expected to be 0 or 1. | ||
| func UniverseAtPos(c *cli.Command, pos int) { |
There was a problem hiding this comment.
Do you need to export it?
There was a problem hiding this comment.
Oh, thanks. I forgot the meaning of the capital letters. Fixed in 9801c78
cli/command/universeN.go
Outdated
| } | ||
| } | ||
|
|
||
| c.Action = prependArgParsingToAction(c, "universe", func(ctx *cli.Context) (string, error) { |
There was a problem hiding this comment.
see comment about prependArgParsingToAction
- rename to universeAtPos
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reading through the
clifolder contents I thought that the command utils were a little messy and had lot of duplicate code. I am not very experienced in Go so I am not familiar with the conventions, however, I think I made some improvements.I thought about adding a commit with more comments for some of the individual commands but those actually have a decent amount of documentation or the function names are self explanatory.
This isn't exactly what I have been asked for but I think it demonstrates my understanding of what I studied and it might even be useful/worth merging.