-
Notifications
You must be signed in to change notification settings - Fork 882
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
GODRIVER-2154 Audit code generated by operationgen and remove operationgen #809
GODRIVER-2154 Audit code generated by operationgen and remove operationgen #809
Conversation
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
// not use this file except in compliance with the License. You may obtain | ||
// a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello.go
was missing a license for some reason (the reason being I made the file and forgot to add the license).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the general procedure deciding when a file should have a license? I'm not super familiar with licensing open source software but it seems pretty important to ensure it's being used the way we expect it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A great question hahah. I'm also not super familiar with our licensing policy, but this copyright notice should probably appear in every file written by us and available to users of the repo. We're certainly missing the copyright on some files (here's just one example). If it's important for us to have copyright on every file, I can file a ticket for auditing the code base (@kevinAlbs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the JIRA ticket and it makes sense that we wouldn't want to ignore errors. What was the main motivation for using operationgen
in the first place (besides code generation)? Does editing code generated by operationgen
break it somehow?
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
// not use this file except in compliance with the License. You may obtain | ||
// a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the general procedure deciding when a file should have a license? I'm not super familiar with licensing open source software but it seems pretty important to ensure it's being used the way we expect it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just left a few questions 👍
GODRIVER-2154
The Go driver used to rely on an internal code generation tool called
operationgen
to create the operation-specific files underx/mongo/driver/operation
. We no longer rely on this tool and now edit these files by hand.This PR does a number of things:
build[type]Result
functions in operations underx/mongo/driver/operation
and correctly returns errors (fixes a number of bugs created by a faultyoperationgen
template).// Code generated by operationgen. DO NOT EDIT.
notes at the top of files inx/mongo/driver/operation
.*.toml
files inx/mongo/driver/operation
.cmd/operationgen
andx/mongo/driver/drivergen
.x/mongo/driver/operation
in.golangci.yml
.The
operationgen
also used a dependency calledpackr
that I believe we can now remove. To avoid bloating this PR, I've filed GODRIVER-2234 to do that separately.