search

Factual / drake

Data workflow tool, like a "Make for data"
Other
1.48k stars 110 forks source link

Make sure should-build? is working with true file paths, intead of th… #195

Closed chen-factual closed 9 years ago

chen-factual commented 9 years ago

…ose with the optional ("?") flag. #194

cc @dirtyvagabond @amalloy Fix for #194, but as mentioned in ticket, full general treatment would be to make FS module aware of optional files.

amalloy commented 9 years ago

Why does make-file-stats return a struct where :path and :file are both paths representing a file, one of which is useable and the other might have a ? in front of it? I think rather than just switching this usage site to use the correct names, we should change make-file-stats so that the names in it make sense: that will lead to fewer errors like this in the future.

chen-factual commented 9 years ago

My original intention for having :file hold the string with "?" was so if we end up passing around this object, we can easily refer to what the user input as the original file (for logging purposes, for instance). I ended up using this map less than anticipated because Drake everywhere just uses a file string, and I didn't want to go around changing all of that.

So, to summarize, :file isn't currently needed, but if decide to use this file map struct everywhere, holding the original user file specification may be useful.

amalloy commented 9 years ago

I agree having them both is useful; I was arguing that the names are awful and confusing. On Aug 19, 2015 3:49 PM, "Chen Guo" notifications@github.com wrote:

My original intention for having :file hold the string with "?" was so if we end up passing around this object, we can easily refer to what the user input as the original file (for logging purposes, for instance). I ended up using this map less than anticipated because Drake everywhere just uses a file string, and I didn't want to go around changing all of that.

So, to summarize, :file isn't currently needed, but if decide to use this file map struct everywhere, holding the original user file specification may be useful.

— Reply to this email directly or view it on GitHub https://github.com/Factual/drake/pull/195#issuecomment-132815802.

chen-factual commented 9 years ago

So is this pull request good? We've talked about the future but not this patch, I wanna get us back on track.

amalloy commented 9 years ago

No, I meant that in the present, in this pull request, I would like the keys in the map produced by make-file-stats to be fixed so that they make sense. If I'd realized what was stored under :path and :file when make-file-stats was added, I would have asked for the names to be fixed then; but now that it's been brought to my attention I don't want to accept a pull request that includes confusing code by perpetuating these names.

amalloy commented 9 years ago

I sympathize with it being hard to come up with good names for these two closely-related attributes, because the difference between the two of them is so thin: :filename and :filename-with-maybe-leading-? is a bit of a mouthful. Why not just save one of them, and then derive the other from the presence of :optional or something? For example you could save just the name without ? in it, under :filename, and then when you need the original name you call (original-filename filestat), instead of just (:path filestat), and it can just throw a ? in front.

chen-factual commented 9 years ago

I'd prefer to keep both around, because I can see having to call original-filename as just one more thing to remember that can be easily forgotten, and it can become one more source of error when you mis-construct the user provided name.

What about using :path and :raw-name? The "raw" should imply that this is a name that shouldn't be used directly.

amalloy commented 9 years ago

:+1:

chen-factual commented 9 years ago

Thanks Alan. Made the change, I'm merging.