-
Notifications
You must be signed in to change notification settings - Fork 305
start of filesystem impl conversion to kotlin #358
base: feature/kotlin_conversion
Are you sure you want to change the base?
Conversation
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.
Great job overall! I think the Util
class has been reviewed on a previous PR! It can also be an object
instead of class, since it does not seem to retain any state!
import io.reactivex.Observable | ||
|
||
|
||
class FSAllEraser(internal val fileSystem: FileSystem) : DiskAllErase { |
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 sure if this was intended to have package level visibility in the previous implementation, put it can be private I think!
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.
done!
* Make sure to have keys containing same data resolve to same "path" | ||
* @param <T> key type | ||
</T> */ | ||
open class FSWriter<T>(internal val fileSystem: FileSystem, internal val pathResolver: PathResolver<T>) : DiskWrite<BufferedSource, T> { |
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.
Same here it can be private I think!
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.
done!
open class FSWriter<T>(internal val fileSystem: FileSystem, internal val pathResolver: PathResolver<T>) : DiskWrite<BufferedSource, T> { | ||
|
||
override fun write(key: T, data: BufferedSource): Single<Boolean> { | ||
return Single.fromCallable { |
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.
You can lift it to assignment!
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.
done!
|
||
class FSAllEraser(internal val fileSystem: FileSystem) : DiskAllErase { | ||
override fun deleteAll(path: String): Observable<Boolean> { | ||
return Observable.fromCallable { |
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.
You can lift it to assignment!
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.
done!
fun create(fileSystem: FileSystem, | ||
expirationDuration: Long, | ||
expirationUnit: TimeUnit): RecordPersister { | ||
return RecordPersister(fileSystem, expirationDuration, expirationUnit) |
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.
You can lift it to assignment!
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.
done!
class SourceAllPersister @Inject | ||
constructor(fileSystem: FileSystem) : AllPersister<BufferedSource, BarCode> { | ||
|
||
internal val sourceFileAllReader: FSAllReader |
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.
All these val
s can be private and by lazy
:)
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.
done!
fun getRecordState(barCode: BarCode, | ||
expirationUnit: TimeUnit, | ||
expirationDuration: Long): RecordState { | ||
return fileSystem.getRecordState(expirationUnit, expirationDuration, SourcePersister.pathForBarcode(barCode)) |
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.
You can lift it to assignment!
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.
done!
|
||
@Throws(FileNotFoundException::class) | ||
override fun readAll(path: String): Observable<BufferedSource> { | ||
return sourceFileAllReader.readAll(path) |
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.
You can lift it to assignment! Similarly on the following methods!
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.
done!
open class SourcePersister @Inject | ||
constructor(fileSystem: FileSystem) : Persister<BufferedSource, BarCode> { | ||
|
||
internal val sourceFileReader: SourceFileReader |
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 be private and by lazy
previously!
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.
done!
@@ -35,7 +35,7 @@ private SourcePersisterFactory() { | |||
if (root == null) { | |||
throw new IllegalArgumentException("root file cannot be null."); | |||
} | |||
return RecordPersister.create(FileSystemFactory.create(root), expirationDuration, expirationUnit); | |||
return RecordPersister.Companion.create(FileSystemFactory.create(root), expirationDuration, expirationUnit); |
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.
You can mark .create()
with @JvmStatic
if this is going to remain a Java file.
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 did not know that! thanks! :) i'm trying to keep the pr's small for the moment :) so i'm converting in batches.
No description provided.