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 log that is always generated for annotation fields. #652

Merged
merged 2 commits into from
Apr 22, 2022
Merged

Conversation

robshakir
Copy link
Contributor

 * (M) ytypes/container.go
  - Since schemas that are generated with annotations will always
    generate the log that says that unmarshal into them is unsupported
    the log message produced is extremely low value to a user. To this
    end - just remove it.

@wenovus -- I concluded that it was just higher-value to remove this entirely. Given that the user knows that they generated the code with annotations, then I'm not sure they ever will have value from this log.

An alternative would be to check that the annotation field was populated, and then produce this log, but I think this seems like we should just implement unmarshalling annotations. I can look at this as a future task.

 * (M) ytypes/container.go
  - Since schemas that are generated with annotations will always
    generate the log that says that unmarshal into them is unsupported
    the log message produced is extremely low value to a user. To this
    end - just remove it.
@robshakir robshakir requested a review from wenovus April 22, 2022 17:18
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0007%) to 90.844% when pulling 268b94d on log into 473d130 on master.

Copy link
Collaborator

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

Makes sense, just outputting it unconditionally seems like more spam than value.

@wenovus
Copy link
Collaborator

wenovus commented Apr 22, 2022

@robshakir How would we like to release ygot right now? ygot releases look to be blocked by #648, should we wait until its resolution or revert #638?

@robshakir
Copy link
Contributor Author

I suggest that we rollback #638 -- I think that will unblock changes, whilst remaining backwards compatible.

Once we resolve the conversation in #648 (which today will contain #638) then we can choose the fate of that code based on that conclusion.

WDYT?

@robshakir robshakir merged commit f9339e1 into master Apr 22, 2022
@robshakir robshakir deleted the log branch April 22, 2022 17:31
@wenovus
Copy link
Collaborator

wenovus commented Apr 22, 2022

SGTM.

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.

3 participants