Adding clean as a default task #253
Conversation
|
First time submitting a PR for this project. I have a few questions:
|
|
Also, one quirk is that clean does not clean up after itself. So after running clean you are left with |
|
Fixes #227 |
main/src/mill/main/MainModule.scala
Outdated
| def clean(targets: String*) = mill.T.command { | ||
| val result = targets match { | ||
| case Nil => | ||
| List(ammonite.ops.%%('rm, "-rf", OutDir)(ammonite.ops.pwd).out.string.trim()) |
There was a problem hiding this comment.
probably should use ammonite.ops.rm here
There was a problem hiding this comment.
Aw okay didn't even know that was a thing.
There was a problem hiding this comment.
I know it sounds silly, but maybe we should do some kind of validation on the paths to be cleaned so that we make sure to avoid this kind of scenario even if the code changes in future?
There was a problem hiding this comment.
Good point. I did note that other functions in this file do the following:
val resolved = RunScript.resolveTasks(
mill.main.ResolveMetadata, evaluator, targets, multiSelect = true
)
And then I extract the names from resolved. Would that be sufficient? I will play with it later.
There was a problem hiding this comment.
would it not be enough to validate that what you pass to rm -rf contains out?
There was a problem hiding this comment.
And maybe that the parent folder contains build.sc?
There was a problem hiding this comment.
Yes you're right. I guess I'm wondering if there is also value is validating that the passed targets are indeed valid targets.
There was a problem hiding this comment.
You probably want to convert the path to a absolute path first before applying tests.
main/src/mill/main/MainModule.scala
Outdated
| case Nil => | ||
| List(ammonite.ops.%%('rm, "-rf", OutDir)(ammonite.ops.pwd).out.string.trim()) | ||
| case _ => | ||
| targets.map(cleanTarget => ammonite.ops.%%('rm, "-rf", s"$OutDir/$cleanTarget")(ammonite.ops.pwd).out.string.trim()) |
There was a problem hiding this comment.
This won't work for targets like foo.bar.baz
There was a problem hiding this comment.
Yeah I wanted to ask about this first to confirm that it is desirable. Will mark as WIP until I've added this.
msoule
changed the title
msoule
changed the title
| */ | ||
| def clean(targets: String*) = mill.T.command { | ||
| val result = Try { | ||
| if (ammonite.ops.ls(ammonite.ops.pwd).contains(ammonite.ops.pwd / BuildFile)) { |
There was a problem hiding this comment.
This can just be exists(pwd / "build.sc")
There was a problem hiding this comment.
You are doing validation on ammonite.ops.pwd instead of the paths you are going to delete
| remove(List(ammonite.ops.pwd / OutDir)) | ||
| case _ => | ||
| remove(targets | ||
| .map(_.replace(".", "/")) |
There was a problem hiding this comment.
You should use resolveDestPaths (https://github.com/lihaoyi/mill/blob/master/core/src/mill/eval/Evaluator.scala#L342) to figure out what targets to clean, to ensure it stays in sync with any future changes
| * Deletes the given targets from the out directory. Providing no targets will clean everything. | ||
| */ | ||
| def clean(targets: String*) = mill.T.command { | ||
| val result = Try { |
There was a problem hiding this comment.
No idea why you are wrapping this in a Try; you just need to return a simple Result object
There was a problem hiding this comment.
The ammonite functions call things which can throw exceptions. Is it acceptable to let those exceptions be thrown?
| } | ||
|
|
||
| private def remove(paths: List[Path]): Result[String] = { | ||
| paths.filter(ammonite.ops.exists).foreach(ammonite.ops.rm) |
There was a problem hiding this comment.
You don't need the filter; rm already just ignores a path if there' nothing to remove
|
This should have at least one unit test somewhere before merging |
|
@lihaoyi Where are the unit tests for the other functions in this file? Are there any? |
|
Also @lihaoyi Can you explain the build failures I'm seeing. The same test passes locally. |
|
Closing due to inactivity |
|
@lihaoyi Can you answer some of the questions I have above? |
No description provided.