-
Notifications
You must be signed in to change notification settings - Fork 141
fix and enforce lint #1429
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
fix and enforce lint #1429
Conversation
|
||
.PHONY: build | ||
build: $(BUILD)/fmt ## ensure all packages build | ||
build: $(BUILD)/lint ## ensure all packages build |
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.
can we run both?
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.
it's already running both. lint depends on fmt.
This is NOT a breaking change. Some files are touched but no API changes.
|
) | ||
|
||
type ( | ||
// Worker hosts workflow and activity implementations. |
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.
not following, where did this change come from? Are they intended to be part of the same lint PR?
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.
ah, I missed that in the diff description earlier. What's the intent behind the copy?
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.
these were defined in worker/worker.go. it seems they are moved here but not aliased in the previous place
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.
Yes, it's copied from worker/worker.go
. Basically internal package is missing this interface.
Alternatively we can use reference which has caused issues in go tooling (like mock) before.
Worker = internal.Worker
internal/internal_worker_test.go
Outdated
w.registry = newRegistry() | ||
assert.Empty(t, w.registry.getRegisteredActivities()) | ||
assert.Empty(t, w.registry.GetRegisteredWorkflowTypes()) | ||
// assert.Empty(t, w.GetRegisteredActivities()) |
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.
should these be asserted or cleaned up?
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.
should be asserted. this is a miss
What changed?
Why?
introduce best practice
How did you test it?
unit test
Potential risks
This is NOT a breaking change. Some files are touched but no API changes.
internal/worker.go
added new Worker interface (copied from top level) and changed NewWorker() return. This NewWorker API is used in the top level and there is NO change in the APIinternal/workflow.go
only comment change to ignore the linterI've copied the template anyway to avoid CI error
Detailed Description
[In-depth description of the changes made to the interfaces, specifying new fields, removed fields, or modified data structures]
Impact Analysis
Testing Plan
Rollout Plan