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

Expose JavaMilli to the whole sbt package #139

Merged
merged 1 commit into from Apr 6, 2018

Conversation

Projects
None yet
3 participants
@dwijnand
Copy link
Member

commented Apr 5, 2018

Allows for reuse, such as in sbt/sbt#4067.

@dwijnand dwijnand added the in progress label Apr 5, 2018

@dwijnand dwijnand requested review from cunei and eed3si9n Apr 5, 2018

@@ -56,7 +56,7 @@ private abstract class StatInt(size: Int, mtimeOffset: Int, mtimensecOffset: Int
def getModifiedTimeNative = (buffer.getInt(mtimeOffset), buffer.getInt(mtimensecOffset))
}

private abstract class Milli {
private[sbt] abstract class Milli {

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Apr 5, 2018

Member

I am of two minds on this one.
I thought we discussed this type of things previously and said we should keep internal code to each modules, and essentially copy-paste so they are less coupled.

This comment has been minimized.

Copy link
@cunei

cunei Apr 5, 2018

Contributor

I see nothing wrong in exposing internal code to [sbt], it is still confined. Code duplication, however, is almost never a good idea.
This PR has my LGTM

This comment has been minimized.

Copy link
@dwijnand

dwijnand Apr 5, 2018

Author Member

I don't mind making the whole thing public either.

This comment has been minimized.

Copy link
@dwijnand

dwijnand Apr 5, 2018

Author Member

by "whole thing" I mean JavaMilli, btw.

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Apr 5, 2018

Member

Making it public is better I think. Let's move it to sbt.io package.

@dwijnand dwijnand requested a review from cunei Apr 6, 2018

@dwijnand

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

done. please re-review @cunei and @eed3si9n.

@eed3si9n
Copy link
Member

left a comment

LGTM pending Travis

Expose JavaMilli into the sbt.io package
Allows for reuse, such as in sbt/sbt#4067.
@cunei

cunei approved these changes Apr 6, 2018

Copy link
Contributor

left a comment

Looks ok!

@dwijnand dwijnand merged commit b35ee7d into sbt:1.1.x Apr 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dwijnand dwijnand deleted the dwijnand:expose-JavaMilli branch Apr 6, 2018

@dwijnand dwijnand added this to the 1.1.6 milestone Apr 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.