-
Notifications
You must be signed in to change notification settings - Fork 28
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
chore(lint): address existing linter issues #1218
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1218 +/- ##
=======================================
Coverage 56.87% 56.87%
=======================================
Files 599 599
Lines 32856 32869 +13
=======================================
+ Hits 18687 18695 +8
- Misses 13535 13538 +3
- Partials 634 636 +2
|
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 remove this too, then?
Line 171 in 31e3eb4
- dupl |
|
||
globals, err := starlark.ExecFileOptions(syntax.LegacyFileOptions(), thread, "templated-base", tmpl, predeclared) |
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.
im assuming its not breaking, but how impactful is this change?
globals, err := starlark.ExecFileOptions(syntax.LegacyFileOptions(), thread, "templated-base", tmpl, predeclared)
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 just using what the now deprecated function has. I think we can investigate turning some of those options on, but they're all set to false right now
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.
awesome
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.
thanks!
A few things of note:
dupl
. This rule was really only pointing out false positives. I suppose you could argue there could be some convoluted abstraction to make step and service not copy so much code, but I don't think that would be worth it. It's really just noise right now.int32
for DB types does not play well withint
.int
will expand based on system, so converting it toint32
does not make much sense, especially when we are usingINTEGER
for the DB table declaration for bothint32
andint64
. So I just made allint
convert toint64
for the DB.contextcheck
was pretty particular about usingdatabase.FromContext(c)
from inside helper functions, so I just grabbed the interface and passed that along instead.uint64
for Starlark exec limit cannot be translated to a sql type, so I moved the conversion for that to right before we pass it along to Starlark.golang-ci lint ✅ 🤞