Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Clean up all but the transient warnings #708

Merged
merged 2 commits into from
Jan 17, 2017
Merged

Conversation

johnynek
Copy link
Collaborator

I removed all but warnings like this:

[warn] /Users/oscar/oss/summingbird/summingbird-scalding/src/main/scala/com/twitter/summingbird/scalding/store/VersionedBatchStore.scala:108: no valid targets for annotation on value injection - it is discarded unused. You may specify targets with meta-annotations, e.g. @(transient @param)
[warn]   implicit @transient injection: Injection[(K2, V2), (Array[Byte], Array[Byte])], override val ordering: Ordering[K])

it seems like this pattern that we have been cargo-culting may not be actually doing anything, but I didn't want to take the risk (yet, but we should fix it).

All the other seemed easy to fix without doing much change to the code.

I also disabled scalariform, which is annoying in that it changes the code under you. I think we should set up scalafmt as a CI check and a command in the build to get code formatting.

Copy link
Collaborator

@piyushnarang piyushnarang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but not super familiar with the SB code, so would be nice to get Pankaj's comments too.

implicit val timeValueSemigroup: Semigroup[(Timestamp, V)] =
IteratorSums.optimizedPairSemigroup[Timestamp, V](1000)
commutativity: Commutativity): TypedPipe[(LTuple2[K1, BatchID], (Timestamp, V1))] = {
implicit val timeV1alueSemigroup: Semigroup[(Timestamp, V1)] =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeV1alueSemigroup, rename to timeV1Semigroup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeValueSemigroup seems ok, the 1 got introduced by search replace I suspect.

@johnynek
Copy link
Collaborator Author

johnynek commented Jan 13, 2017 via email

@codecov-io
Copy link

codecov-io commented Jan 13, 2017

Current coverage is 71.02% (diff: 83.01%)

Merging #708 into develop will decrease coverage by 0.07%

@@            develop       #708   diff @@
==========================================
  Files           141        141          
  Lines          3461       3469     +8   
  Methods        3263       3274    +11   
  Messages          0          0          
  Branches        198        195     -3   
==========================================
+ Hits           2461       2464     +3   
- Misses         1000       1005     +5   
  Partials          0          0          

Powered by Codecov. Last update db801c2...9077b28

@johnynek
Copy link
Collaborator Author

@pankajroark good to go with the new name?

@pankajroark
Copy link
Contributor

👍

@johnynek
Copy link
Collaborator Author

the 2.10 build keeps timing out. Not really sure what is going on. I'm hoping the #699 will just fix this.

I can't see how changing these warnings can possibly be related.

@ttim
Copy link
Collaborator

ttim commented Jan 17, 2017

@johnynek It's green now, I think it's better to merge this change before Storm's one.

@johnynek johnynek merged commit 44fb5c9 into develop Jan 17, 2017
@johnynek johnynek deleted the oscar/clean-warnings branch January 17, 2017 22:12
@johnynek
Copy link
Collaborator Author

@ttim maybe some issue with travis? Maybe just flakey tests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants