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

Remove unnecessary print statement in schema apis #4355

Merged
merged 1 commit into from
May 23, 2017

Conversation

vjsamuel
Copy link
Contributor

No description provided.

@exekias
Copy link
Contributor

exekias commented May 19, 2017

jenkins test it

@@ -128,7 +128,6 @@ func StrFromNum(key string, opts ...schema.SchemaOption) schema.Conv {
func toStr(key string, data map[string]interface{}) (interface{}, error) {
emptyIface, err := common.MapStr(data).GetValue(key)
if err != nil {
fmt.Println(err)
return "", fmt.Errorf("Key %s not found", key)
Copy link
Member

Choose a reason for hiding this comment

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

The fmt must definitively be removed. But we should add it to the error that is returned:

return "", fmt.Errorf("Key %s not found: %s", key, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin, we do return an error here. it can be left upto the consumer to decide if this needs to be logged or not. for instance I got this error when I had set the Conv to optionally extract a field out. It should be logging errors for such cases. that would just fill up my log file because there may be many fields that are set to optional. thought?

Copy link
Member

Choose a reason for hiding this comment

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

I only mean to include the error in the error message.

@@ -43,7 +43,7 @@ https://github.com/elastic/beats/compare/v6.0.0-alpha1...master[Check the HEAD d
- Fix a debug statement that said a module wrapper had stopped when it hadn't. {pull}4264[4264]
- Use MemAvailable value from /proc/meminfo on Linux 3.14. {pull}4316[4316]
- Fix panic when events were dropped by filters. {issue}4327[4327]

- Remove unnecessary print statement in schema apis {pull}4355[4355]
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a newline afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@andrewkroh
Copy link
Member

jenkins, test it

1 similar comment
@ruflin
Copy link
Member

ruflin commented May 22, 2017

jenkins, test it

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Seems like something went wrong during rebasing as it now contains also lots of other commits and changes.

@@ -128,7 +128,6 @@ func StrFromNum(key string, opts ...schema.SchemaOption) schema.Conv {
func toStr(key string, data map[string]interface{}) (interface{}, error) {
emptyIface, err := common.MapStr(data).GetValue(key)
if err != nil {
fmt.Println(err)
return "", fmt.Errorf("Key %s not found", key)
Copy link
Member

Choose a reason for hiding this comment

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

I only mean to include the error in the error message.

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

WFG

@ruflin
Copy link
Member

ruflin commented May 22, 2017

jenkins, test it

@ruflin ruflin merged commit a92e230 into elastic:master May 23, 2017
@vjsamuel vjsamuel deleted the remove_unnecessary_print branch March 5, 2018 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants