GetStdinFile functionality moved from score compose #85
Conversation
rabelmervin
changed the title
|
Hi @astromechza @mathieu-benoit Can you please review it.... |
uriget/uriget.go
Outdated
| if rawUri == "-" { | ||
| return getStdinFile(ctx) | ||
| } |
There was a problem hiding this comment.
Instead of handling "-" separately in the command, consider adding support for it directly in GetFile, using a case "-" inside uriget.go
This would keep the logic consistent with other URI options and improve maintainability
There was a problem hiding this comment.
➕ @rabelmervin please move the rawUri == "-" check down into o.getFile as it's a variant of the file reading code - I don't want to have extra branches not related to the scheme checking here in the top-level GetFile function.
uriget/uriget.go
Outdated
| func getStdinFile(ctx context.Context) ([]byte, error) { | ||
| // Check if stdin is being piped | ||
| stat, err := os.Stdin.Stat() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Check if stdin is a pipe | ||
| if (stat.Mode() & os.ModeCharDevice) == 0 { | ||
| return io.ReadAll(os.Stdin) | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("no stdin data provided") | ||
| } | ||
|
|
There was a problem hiding this comment.
It's a good practice to document the codebase but instead of these line comments you can add a small description here
Also, add a small test at https://github.com/score-spec/score-go/blob/main/uriget/example_test.go
There was a problem hiding this comment.
➕ @rabelmervin please add a unit test that temporarily assigns a file to os.Stdin and checks that we can read from it.
A manual test of this function may be useful too if you can.
|
Thanks, @rabelmervin |
uriget/uriget.go
Outdated
| if rawUri == "-" { | ||
| return getStdinFile(ctx) | ||
| } |
There was a problem hiding this comment.
➕ @rabelmervin please move the rawUri == "-" check down into o.getFile as it's a variant of the file reading code - I don't want to have extra branches not related to the scheme checking here in the top-level GetFile function.
uriget/uriget.go
Outdated
| func getStdinFile(ctx context.Context) ([]byte, error) { | ||
| // Check if stdin is being piped | ||
| stat, err := os.Stdin.Stat() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Check if stdin is a pipe | ||
| if (stat.Mode() & os.ModeCharDevice) == 0 { | ||
| return io.ReadAll(os.Stdin) | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("no stdin data provided") | ||
| } | ||
|
|
There was a problem hiding this comment.
➕ @rabelmervin please add a unit test that temporarily assigns a file to os.Stdin and checks that we can read from it.
A manual test of this function may be useful too if you can.
Signed-off-by: rabelmervin <[email protected]>
Signed-off-by: rabelmervin <[email protected]>
Signed-off-by: rabelmervin <[email protected]>
|
Thanks @astromechza @mathieu-benoit for the opportunity. Why can't I make it double. |
|
@astromechza I would like to connect with you on socials. |
Description
This pull request moves the
GetStdinFilefunction from the score-compose repository to the score-go repository under the uriget package.Changes Made
Moved
GetStdinFileto score-go:The
GetStdinFilefunction is now located in score-go/uriget/uriget.go.The
GetStdinFileis now integrated as a part ofGetFileThis function reads from stdin and returns the content.