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

Adds `IO.getModifiedTimeOrZero(file)` and `IO.setModifiedTimeOrFalse(file)` #106

Merged
merged 8 commits into from Dec 22, 2017

Conversation

Projects
None yet
4 participants
@cunei
Contributor

cunei commented Dec 18, 2017

Ref #92

There are just too many instances in which sbt's code relies on
the lastModified/setLastModified semantics, so instead of moving
to get/setModifiedTime, we use new IO calls that offer the new
timestamp precision, but retain the old semantics.

cunei added some commits Dec 18, 2017

Revert *ModifiedTime() calls to *lastModified*() calls
There are just too many instances on which sbt's code relies on
the lastModified/setLastModified semantics, so instead of moving
to get/setModifiedTime, we use new IO calls that offer the new
timestamp precision, but retain the old semantics.

@cunei cunei requested a review from dwijnand Dec 18, 2017

@cunei cunei added the in progress label Dec 18, 2017

@cunei cunei requested a review from eed3si9n Dec 18, 2017

Small optimization for MacMilli
The attr descriptor can be reused, so there is no need to
reallocate and reinitialize it every time.
@cunei

This comment has been minimized.

Show comment
Hide comment
@cunei

cunei Dec 20, 2017

Contributor

We had a long discussion concerning these pull requests, and developed a compromise that, although not particularly pretty in the interim, will eventually get us to a single set of calls with the Try semantics. In short:

  • The zero-returning variants are problematic, and we will move away from them eventually, but not now
  • The exception-throwing variants are better, but having the same prototype as the ones above may be confusing
  • The Try version is preferable, but we cannot add it right away, otherwise we end up with too many calls in IO (get/set/copy, in three variants, would mean having nine different calls in IO)

So:

  • We are going to stick with the zero-returning semantics for now (equivalent to java.io.File's lastModified/setLastModified)
  • In order to make it clear that the methods may deal with zero timestamps, however, in IO we will use for them the names getModifiedTimeOrZero() and setModifiedTimeOrFalse() (the existing copyLastModified(), already in the API, will be maintained)
  • The just-added get/set/copyModifiedTime will be deprecated, so that they may be eventually removed.
  • In a future version of sbt/io, once we remove the modifiedTime calls, we will add instead the versions that return Try (or Either), so that the different types can guide us into a migration. The zero-returning variants will be deprecated.

In conclusion, eventually we will have only three get/set/copy calls, based on Try.

Contributor

cunei commented Dec 20, 2017

We had a long discussion concerning these pull requests, and developed a compromise that, although not particularly pretty in the interim, will eventually get us to a single set of calls with the Try semantics. In short:

  • The zero-returning variants are problematic, and we will move away from them eventually, but not now
  • The exception-throwing variants are better, but having the same prototype as the ones above may be confusing
  • The Try version is preferable, but we cannot add it right away, otherwise we end up with too many calls in IO (get/set/copy, in three variants, would mean having nine different calls in IO)

So:

  • We are going to stick with the zero-returning semantics for now (equivalent to java.io.File's lastModified/setLastModified)
  • In order to make it clear that the methods may deal with zero timestamps, however, in IO we will use for them the names getModifiedTimeOrZero() and setModifiedTimeOrFalse() (the existing copyLastModified(), already in the API, will be maintained)
  • The just-added get/set/copyModifiedTime will be deprecated, so that they may be eventually removed.
  • In a future version of sbt/io, once we remove the modifiedTime calls, we will add instead the versions that return Try (or Either), so that the different types can guide us into a migration. The zero-returning variants will be deprecated.

In conclusion, eventually we will have only three get/set/copy calls, based on Try.

cunei added some commits Dec 21, 2017

Enable modified files test
Since #82 is now closed,
enable the previously pending test.
@cunei

This comment has been minimized.

Show comment
Hide comment
@cunei

cunei Dec 21, 2017

Contributor

@dwijnand, since #82 is closed I re-enabled the pending modified files detection test; please review.

Contributor

cunei commented Dec 21, 2017

@dwijnand, since #82 is closed I re-enabled the pending modified files detection test; please review.

@eed3si9n

LGTM. Thanks for your patience on this Toni!

@typesafe-tools

This comment has been minimized.

Show comment
Hide comment
@typesafe-tools

typesafe-tools Dec 22, 2017

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project Reference Commit
sbt pull/3822/head sbt/sbt@94e36a1
zinc pull/470/head sbt/zinc@67d3878
io pull/106/head 77d3ba2
librarymanagement pull/192/head sbt/librarymanagement@72dd006
util pull/136/head sbt/util@0a1bd5a
website 1.x

The result is: SUCCESS
(restart)

typesafe-tools commented Dec 22, 2017

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project Reference Commit
sbt pull/3822/head sbt/sbt@94e36a1
zinc pull/470/head sbt/zinc@67d3878
io pull/106/head 77d3ba2
librarymanagement pull/192/head sbt/librarymanagement@72dd006
util pull/136/head sbt/util@0a1bd5a
website 1.x

The result is: SUCCESS
(restart)

@cunei

This comment has been minimized.

Show comment
Hide comment
@cunei

cunei Dec 22, 2017

Contributor

@eed3si9n The five pull requests are ready for merging now.

Contributor

cunei commented Dec 22, 2017

@eed3si9n The five pull requests are ready for merging now.

@eed3si9n eed3si9n merged commit 63880dd into sbt:1.1.x Dec 22, 2017

1 check passed

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

@eed3si9n eed3si9n changed the title from Revert *ModifiedTime() calls to *lastModified*() calls to Adds `IO.getModifiedTimeOrZero(file)` and `IO.setModifiedTimeOrFalse(file)` Dec 22, 2017

@cunei cunei deleted the cunei:wip-milliBis branch Dec 23, 2017

pending // until fixed https://github.com/sbt/io/issues/82
watchTest(parentDir) {
IO.write(file, "bar")
}

This comment has been minimized.

@dwijnand

dwijnand Jan 2, 2018

Member

This shouldn't've been enabled like this because it fails on HFS+. See https://github.com/sbt/io/pull/102/files#diff-32eb449ec95851dd791288244207a8f8R32.

@dwijnand

dwijnand Jan 2, 2018

Member

This shouldn't've been enabled like this because it fails on HFS+. See https://github.com/sbt/io/pull/102/files#diff-32eb449ec95851dd791288244207a8f8R32.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment