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

Enhancement: in REPL mode allow user to specify path to file for bytes field #280

Merged
merged 5 commits into from
May 24, 2020

Conversation

BenSlabbert
Copy link
Contributor

see Issue: #279

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #280 into master will decrease coverage by 0.09%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
- Coverage   82.57%   82.47%   -0.10%     
==========================================
  Files          57       57              
  Lines        3139     3156      +17     
==========================================
+ Hits         2592     2603      +11     
- Misses        313      316       +3     
- Partials      234      237       +3     

@BenSlabbert
Copy link
Contributor Author

the build is failed because of the additional \r byte in windows files.
will fix if the PR is deemed applicable

@BenSlabbert BenSlabbert force-pushed the enhancement/read-file-in-repl branch from e9e25bb to d6b5032 Compare May 12, 2020 09:50
@@ -1,16 +1,86 @@
package proto_test
package proto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped the _test because I needed access to the internal function I wanted to test.

@ktr0731 ktr0731 assigned ktr0731 and BenSlabbert and unassigned ktr0731 May 16, 2020
@ktr0731 ktr0731 self-requested a review May 16, 2020 16:29
Copy link
Owner

@ktr0731 ktr0731 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in replying.
It's almost perfect! I have only pointed out a few minor points.

@@ -213,7 +217,7 @@ func (f *InteractiveFiller) inputField(dmsg *dynamic.Message, field *desc.FieldD
f.state.color.Next()
default: // Normal fields.
f.prompt.SetPrefix(f.makePrefix(field))
v, err := f.inputPrimitiveField(field)
v, err := f.inputPrimitiveField(descriptor.FieldDescriptorProto_Type(descriptor.FieldDescriptorProto_Type_value[field.GetType().String()]))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
v, err := f.inputPrimitiveField(descriptor.FieldDescriptorProto_Type(descriptor.FieldDescriptorProto_Type_value[field.GetType().String()]))
v, err := f.inputPrimitiveField(field.GetType())

Comment on lines 472 to 475
absPath, err := filepath.Abs(path)
if err != nil {
return nil, err
}
Copy link
Owner

Choose a reason for hiding this comment

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

These lines are unnecessary because ioutil.ReadFile also accepts relative paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't know that :)

@@ -301,7 +305,7 @@ func (f *InteractiveFiller) inputRepeatedField(dmsg *dynamic.Message, field *des

// inputPrimitiveField reads an input and converts it to a Go type.
// If CTRL+d is entered, inputPrimitiveField returns io.EOF.
func (f *InteractiveFiller) inputPrimitiveField(field *desc.FieldDescriptor) (interface{}, error) {
func (f *InteractiveFiller) inputPrimitiveField(fieldType descriptor.FieldDescriptorProto_Type) (interface{}, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

👍

Comment on lines 11 to 13
type testPrompt struct {
input string
}
Copy link
Owner

Choose a reason for hiding this comment

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

Can you embed prompt.Prompt and remove methods other than Input?

Suggested change
type testPrompt struct {
input string
}
type testPrompt struct {
prompt.Prompt
input string
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great suggestion thank you!

Copy link
Owner

@ktr0731 ktr0731 left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks for your contribution!

@ktr0731 ktr0731 merged commit b5f289f into ktr0731:master May 24, 2020
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.

2 participants