feat Touch #158
feat Touch #158
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
==========================================
+ Coverage 61.50% 62.01% +0.50%
==========================================
Files 29 30 +1
Lines 2824 2959 +135
==========================================
+ Hits 1737 1835 +98
- Misses 1087 1124 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
That also sounds better to me |
|
I think one of the problems is that the This is why it's currently not possible to do I am not sure how we want to fix this generally. For |
|
Yes! That is what I was thinking as well to copy all of the required functions and take care of it on our end. |
My understanding is that the nushell people libar-ify each uutil command upstream before they integrate it. We could consider doing something similar. |
|
I am not sure what |
PRs like this one: uutils/coreutils#5946 Tbf I thought they went further than this, so my comment didn't make much sense in this discussion. |
|
@Hofer-Julian I like your idea to offload as many things as we can upstream to |
|
Yes, I think we are already benefitting from that! :) I read |
|
The functions from // TODO: this may be a good candidate to put in fsext.rs
/// Returns a PathBuf to stdout.
///
/// On Windows, uses GetFinalPathNameByHandleW to attempt to get the path
/// from the stdout handle.
fn pathbuf_from_stdout() -> UResult<PathBuf> {I'll try to submit a PR for this tomorrow to coreutils. |
|
Awesome, thanks! I went to test it out on Windows, but unfortunately it didn't build: So we need to do two things:
|
|
Ahh yes! I missed that. I have added windows CI in #160. |
|
I think there is no |
The test builder itself creates a temporary directory and adds it as a env var before running the test. I am trying to recreate the error locally with cross but everything passes on Windows. |
|
I think I got it. The behaviour of |
|
So, based on my understanding, there is an issue here with the way testing is being done. × cannot touch C:/non_existent_dir/non_existent.txt: No such file or
│ directoryThe existence of |
|
What is the problem with If the issue is absolute paths, we can filter them, we do that in LFortran so that we can test stacktraces. |
|
@certik Basically, the way testing is done right now is by simply checking if the stderr is exactly the same as the expected stderr or if it contains the expected string. For example in the case of the failing test and the reason I haven't really been able to reproduce this locally: × cannot touch C:/non_existent_dir/non_existent.txt: No such file or
│ directoryWhereas locally this is just × cannot touch C:/non_existent_dir/non_existent.txt: No such file or directorySo when we check wether the stderr contains the string "No such file or directory" it raises an error because actually there is a Note: this is a bug with the way testing is being done. Not with touch. Touch's behaviour is as expected. |
|
I see. Who is inserting the |
|
I believe that is just how miette fancy feature works. It prints each line starting with a |
Yeah, let's adapt the testing approach. Doing it in a separate PR sounds alright. |
|
This was much easier to handle than expected. |
|
After the merge conflicts are resolved, we can merge this from my side |
|
conflicts are resolved now. |
Closes #144.
However, there is one thing. When using
uu_touchfor this, we are not passing theShellStateto theuumainfunction. Now, I have gone over the args, addedstate.cwdto the filenames (if not absolute) and then passed touumain. The other option would be to copy and use the function insideuu_touch. I like the second option better giving us better control over this. What currently is implemented works but seems a bit hacky but let me know what you think!