add: add command 'add' that adds dependents to dependents.txt#1702
add: add command 'add' that adds dependents to dependents.txt#1702amishhaa wants to merge 8 commits intoopen-telemetry:mainfrom
Conversation
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1702 +/- ##
==========================================
+ Coverage 64.19% 64.43% +0.24%
==========================================
Files 69 71 +2
Lines 3463 3509 +46
==========================================
+ Hits 2223 2261 +38
- Misses 998 1002 +4
- Partials 242 246 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
|
|
||
| // addCmd represents the add command | ||
| func addCmd() *cobra.Command { | ||
| var path string |
There was a problem hiding this comment.
Nit: Cobra's examples seem to move this to a top-level variable, see e.g. https://cobra.dev/docs/how-to-guides/working-with-flags/#make-a-flag-required
This pattern is fine, but I would rather follow what Cobra does in its examples
| err := internal.AddDependentsFromFile(path) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
If you want to be a bit more compact you can do the following
| err := internal.AddDependentsFromFile(path) | |
| if err != nil { | |
| return err | |
| } | |
| if err := internal.AddDependentsFromFile(path); err != nil { | |
| return err | |
| } |
| err := internal.AddDependents(args) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Same here!
| err := internal.AddDependents(args) | |
| if err != nil { | |
| return err | |
| } | |
| if err := internal.AddDependents(args); err != nil { | |
| return err | |
| } |
| var err error | ||
| var out string | ||
|
|
||
| out, err = runCobra(t, "add", "--help") |
There was a problem hiding this comment.
You can define the variables implicitly and Go will figure the types out
| var err error | |
| var out string | |
| out, err = runCobra(t, "add", "--help") | |
| out, err := runCobra(t, "add", "--help") |
| var err error | ||
| var out string |
There was a problem hiding this comment.
Same here! (only use := on the first out, err definition, then you use =)
| func (w *Workspace) AddDependent(dependent string) error { | ||
| f, err := os.OpenFile(w.dependentsPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, fileReadWrite) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open dependents.txt: %w", err) | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
I think it would be more convenient if you add the dependents to an internal list and then have a CommitToFile method that writes them to the file. Or you could even pass a dependents []string directly to this function.
There are two reasons for this:
- It is more performant: you avoid closing and re-opening the file every time and in general doing bigger writes rather than many smaller writes is more performant. The programming language and the operating system may do some of this batching into big writes for you, but it is good practice to do it yourself when it makes sense.
- If at some point you need to add more structure to your list of dependents, it is much easier and more performant to do so with some Go struct that you modify than reading the file and writing to it again. Reading from your program's struct is much more performant than reading from the disk.
| } | ||
| defer f.Close() | ||
|
|
||
| _, err = fmt.Fprintln(f, dependent) |
There was a problem hiding this comment.
I think it would be better to write your list of dependents on JSON format. Right now you only need to store each dependent on the list, but if you need to add multiple data about each dependent, it will become harder.
For example, instead of writing each item into a new line, you can define a struct like:
type Dependent struct {
Path string `json:"path"`
}and use the encoding/json/v2 package to marshal and unmarshal it. You can see some examples on how to use it here.
This will become easier to do if you address my comment above.
Part of #1528.
Add command 'add' to write dependents to dependents.txt of workspace.
Usage:
grater add foo/bar --file <file_path>
Update related usage mentions in rootUsage and README to mention this command.
Add AddDependent method in workspace.go as the simplest method to write a raw string to dependents.txt in the workspace.
Add AddDependents method in internal/add.go to write each dependent using ws instance and AddDependentsFromFile method to parse a file and pass it to AddDependents.
Call AddDependentsFromFile and AddDependents from cmdAdd to write dependents.
Add GetDependents to retrieve the written dependents from ws instance.
Add internal.go file to write usage comment for header package internal.