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

target: rework modtime comparison logic #323 #324

Merged
merged 4 commits into from
Dec 22, 2020

Conversation

jasonmf
Copy link
Contributor

@jasonmf jasonmf commented Dec 7, 2020

Within the target package:

  • Separate discovery of newest target timestamp from source inspection
  • Allow source inspection functions to bail out as soon as they hit a timestamp newer than the target
~/p/mage/target$ go test -count=1 .
ok      github.com/magefile/mage/target 0.073s

@DavidGamba
Copy link

I love this separate approach, the only other thing that I would add to this, which is the reason I don't use mage target is because in the logs I want to see a report of the file I found that was modified. Sometimes there are complex dependency trees and knowing what triggered what is helpful.

@jasonmf
Copy link
Contributor Author

jasonmf commented Dec 8, 2020

I started out trying to make it compatible with the upcoming io/fs interfaces to make it easy to write tests that didn't have to touch the filesystem. This would also allow user implementations that return atime, ctime, or other arbitrary times. Ultimately I saw there were already perfectly good unit tests so I got lazy. I might revisit this when io/fs lands and then users can inject whatever funky filesystem behavior they want.

Copy link
Member

@natefinch natefinch left a comment

Choose a reason for hiding this comment

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

Mostly LGTM with a couple tiny tweaks

target/target.go Show resolved Hide resolved
target/target.go Outdated Show resolved Hide resolved
target/newer.go Outdated Show resolved Hide resolved
@jasonmf
Copy link
Contributor Author

jasonmf commented Dec 22, 2020

Requested changes pushed

@natefinch
Copy link
Member

So, coming back to this, now that I better grok why there's an expanded API surface, I'm a little more ok with it. Letting people punch in their own modtime does make the code more flexible.

@natefinch natefinch merged commit 3730191 into magefile:master Dec 22, 2020
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.

3 participants