-
Notifications
You must be signed in to change notification settings - Fork 108
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
Support new scheduled charging #305
Support new scheduled charging #305
Conversation
@@ -1,7 +1,7 @@ | |||
// Code generated by protoc-gen-go. DO NOT EDIT. | |||
// versions: | |||
// protoc-gen-go v1.28.1 | |||
// protoc v3.21.9 | |||
// protoc v5.26.1 |
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.
We're going to have a lot of churn if different people are using different protoc versions to generate these files. Not a big deal, but it does make it harder to tell at a glance if a .pb.go file was modified in a substantive way without a corresponding update to the proto source file.
Things to consider for future PRs:
- Could we generate the pb.go files automatically at merge time?
- Short of that, could we regenerate the pb.go files and check for discrepancies as a pre-merge check?
- If we do the latter, we should include a Dockerfile for building the proto files using a fixed protoc version.
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.
good callout, created #307
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.
Updated to generate with the proper protoc version. Looks like there are still a few unnecessary changes but much better.
8d9e12e
to
f0fa46e
Compare
@sethterashima this deserves a version bump. Do you prefer bumping version as a part of this PR or a followup PR which is deadicated to the version bump? Minor or patch? |
d77b804
to
a46bd5c
Compare
a46bd5c
to
0500fb1
Compare
float longitude = 11; | ||
} | ||
|
||
message PreconditionSchedule { |
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.
nit: can we support generating protobufs in makefile?
https://github.com/teslamotors/vehicle-command/blob/main/Makefile
we do something like this in fleet telemetry as well
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.
See above. created #307 to improve our proto generation.
Backwards compatible API changes should be a minor version bump. I've been stingy with the version bumps in the past, but if we're moving to a bump with each new feature my preference would be to include a version bump in the same PR. Versioning rules are lax in 0.x.x releases, but we should also decide if we're ready to move up to a 1.0.0 release. |
Adds support for weekly charging schedules.
0500fb1
to
08cd2d6
Compare
Bumped to v0.2.0. I would say version bumps are only necessary when changes people will want to use immediately are made. |
Adds support for weekly charging schedules.
Description
Building off of #296
Fixes #290
Type of change
Please select all options that apply to this change:
Checklist:
Confirm you have completed the following steps: