-
Notifications
You must be signed in to change notification settings - Fork 513
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
Update FixAvroCoder to match transformed Avro SCollections #5351
Conversation
.exists { | ||
case TypeSignature(_, _, TypeRef(_, maybeAvroType, _)) => | ||
AvroMatcher.matches(maybeAvroType) | ||
case _ => false |
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 was getting a compiler warning about this line, so added the exhaustive matcher
package fix.v0_14_0 | ||
|
||
import com.spotify.scio.values.SCollection | ||
import com.spotify.scio.avro._ |
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.
why do we add the import here ? It is a false positive ?
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 is needed in this case -- without it, data.map(r => ("foo", r))
throws a Compilation error:
[error] /Users/clairem/scio/scalafix/output-0_14/src/main/scala/fix/v0_14_0/FixAvroCoder16.scala:9:13:
[error] Cannot find an implicit Coder instance for type:
[error]
[error] >> (String, fix.v0_14_0.A)
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 forgot A
is not a generic type but a SpecificRecord
🤦♂️
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.
haha, we probably could have given it a better class name 😅
Our Scalafix rule was missing utility-type classes that contain helper methods for SCollections, like:
This updates the rule to check for method that returns
SCollection[T <: SpecificRecord]
. There's a tiny chance of a false positive if the method doesn't actually invoke any SCollection mapper functions but I don't think that usage is very common. I could update the method to inspect the function body for usage of functions with a Coder bound but I think that would be very error-prone