rm: add verbose output and trim multiple slashes #1988
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Uses the normalize_path used in cargo to strip duplicate slashes With a link to a std rfc rust-lang/rfcs#2208 This fixes uutils#1829 This also touches uutils#1768 but does not attempt to fully solve it
This is ambiguous as we have no precedent for how windows paths should be formatted So I picked the canonical form
sylvestre
reviewed
Apr 1, 2021
|
I noticed that running the test suite on windows that there are unrelated tests failing, but they were failing before my change and since ci is happy I'm assuming its fine. There are also unrelated rustfmt changes that I did not commit because they are not relevant to the change at hand Let me know if there is anything I can do to improve the change as I can test on both platforms |
Seems like I got hit by rust-lang/rustfmt#803 It did not ran formatting on the tests
jhscheer
reviewed
Apr 1, 2021
Since we don't run uucore tests by default I added them to our ci and readme
|
Should be ready to review now :)
|
sylvestre
reviewed
Apr 2, 2021
|
@sylvestre let me know if anything is still missing :) |
sylvestre
reviewed
Apr 5, 2021
| @@ -150,7 +150,7 @@ jobs: | |||
| uses: actions-rs/cargo@v1 | |||
| with: | |||
| command: test | |||
| args: --features "feat_os_unix" | |||
| args: --features "feat_os_unix" -p uucore -p coreutils | |||
There was a problem hiding this comment.
please remove this
Suggested change
| args: --features "feat_os_unix" -p uucore -p coreutils | |
| args: --features "feat_os_unix" |
please create a separate PR for this with the rational
There was a problem hiding this comment.
created #2030 which now blocks this pr :)
sylvestre
reviewed
Apr 5, 2021
| @@ -536,6 +536,17 @@ jobs: | |||
| CARGO_UTILITY_LIST_OPTIONS="$(for u in ${UTILITY_LIST}; do echo "-puu_${u}"; done;)" | |||
| echo set-output name=UTILITY_LIST::${UTILITY_LIST} | |||
| echo ::set-output name=CARGO_UTILITY_LIST_OPTIONS::${CARGO_UTILITY_LIST_OPTIONS} | |||
| - name: Test uucore | |||
sylvestre
reviewed
Apr 5, 2021
| @@ -226,6 +226,11 @@ If you would prefer to test a select few utilities: | |||
| $ cargo test --features "chmod mv tail" --no-default-features | |||
| ``` | |||
|
|
|||
sylvestre
reviewed
Apr 5, 2021
marvin-bitterlich
added a commit
to marvin-bitterlich/coreutils
that referenced
this pull request
Apr 5, 2021
Before this change we never ran tests on uucore itself meaning that is was not possible to test functions of the shared core, only their usage in the different binaries This change adds running uucore to our ci, which will increase coverage for the few doctests that exist and is extracted from uutils#1988 where first tests for uucore will be introduced
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1829
Uses the
normalize_pathused in cargo to strip duplicate slashesWith a link to a std rfc rust-lang/rfcs#2208
This fixes #1829
This also touches #1768
but does not attempt to fully solve it, as I rather have that be a separate pr
The fix to enable any output was to use the recursive version whenever we want to be verbose. #1768 (comment) suggests to anyway run
remove_dir_allafter we do the recursive delete but I feel more tests are neededI also updated the test proposed in #1829 to account for windows having a different normalized path using
\\