Skip to content
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

FTP - attribute enrichment of FTPFile #153 #195

Merged
merged 2 commits into from
Mar 23, 2017

Conversation

svezfaz
Copy link
Contributor

@svezfaz svezfaz commented Feb 18, 2017

Handles #153

Copy link

@juanjovazquez juanjovazquez left a comment

Choose a reason for hiding this comment

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

Some minor things. Looks great!.

val total = result.foldLeft(0L) {
case (acc, next) => acc + next.count
}
val total = result.map(_.count).sum

Choose a reason for hiding this comment

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

this traverses result twice, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given this is a test, I appreciate increased readability over performance. I am not too adamant about it, though :) I can put it back if there is a strong argument

Choose a reason for hiding this comment

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

No strong argument at all. I guess it's just a matter of style.

Copy link
Member

Choose a reason for hiding this comment

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

in a test readability definitely wins over pers in things like these IMHO, thanks!

@@ -4,11 +4,13 @@
package akka.stream.alpakka.ftp
package impl

import org.apache.commons.net.ftp.{FTP, FTPClient}
import org.apache.commons.net.ftp.{FTP, FTPClient, FTPFile}

Choose a reason for hiding this comment

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

blank line

import java.io.InputStream
import java.nio.file.Paths
import java.nio.file.attribute.PosixFilePermissions

Choose a reason for hiding this comment

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

blank line

import akka.stream.alpakka.ftp.RemoteFileSettings.SftpSettings
import com.jcraft.jsch.{ChannelSftp, JSch}

Choose a reason for hiding this comment

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

blank line

import scala.collection.immutable
import scala.util.Try
import java.io.InputStream
import java.nio.file.Paths

Choose a reason for hiding this comment

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

blank line

@@ -7,10 +7,12 @@
import akka.stream.ActorMaterializer;
import akka.stream.Materializer;
import org.apache.mina.util.AvailablePortFinder;

Choose a reason for hiding this comment

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

blank line

@@ -8,7 +8,8 @@ import akka.stream.IOResult
import akka.stream.scaladsl.Source
import akka.util.ByteString
import org.scalatest.concurrent.ScalaFutures
import org.scalatest.{BeforeAndAfter, BeforeAndAfterAll, Matchers, WordSpecLike}
import org.scalatest._

Choose a reason for hiding this comment

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

I'd rather not go with global imports: org.scalatest._

@@ -3,10 +3,14 @@
*/
package akka.stream.alpakka.ftp

import java.nio.file.attribute.PosixFilePermission

Choose a reason for hiding this comment

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

So far, I've been putting the java imports at the end of the import list. Just by convention.

import akka.stream.IOResult
import akka.stream.scaladsl.{Keep, Sink}
import akka.stream.testkit.scaladsl.TestSink
import org.scalatest.time.{Millis, Seconds, Span}

Choose a reason for hiding this comment

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

blank line

import akka.stream.IOResult
import akka.stream.scaladsl.{Keep, Sink}
import akka.stream.testkit.scaladsl.TestSink
import org.scalatest.time.{Millis, Seconds, Span}

import scala.concurrent.duration._

Choose a reason for hiding this comment

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

I'd get rid of the global import on scala.concurrent.duration._

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to have the .millis and .minute conversions from duration package object. would you prefer not to use them?

@svezfaz
Copy link
Contributor Author

svezfaz commented Feb 20, 2017

@juanjovazquez Thanks for reviewing! I updated the imports.

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM!

@ktoso
Copy link
Member

ktoso commented Mar 23, 2017

LGTM, resolved conflicts

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

Successfully merging this pull request may close these issues.

4 participants