Skip to content

tr: Expanding expansion module #2502

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

Merged
merged 69 commits into from
Jan 23, 2022

Conversation

hbina
Copy link
Contributor

@hbina hbina commented Jul 16, 2021

Extending the functionality of tr to possibly be feature-parity with GNU tr.

@hbina hbina force-pushed the hbina-tr-reimplement-expansion branch from f2a228d to 7a03664 Compare July 17, 2021 17:02
@hbina hbina marked this pull request as ready for review July 18, 2021 06:17
@tertsdiepraam tertsdiepraam linked an issue Jul 19, 2021 that may be closed by this pull request
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Great work! This indeed brings us much closer to full compatibility. I've left some suggestions, mostly for cleaning up some of the nom code, but nothing really major.

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

A few more tiny suggestions. You've made some nice changes since I last looked at this. The overall design looks very good!

Copy link
Contributor

@miDeb miDeb left a comment

Choose a reason for hiding this comment

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

There are some errors abuot the spelling. You can disable the spell checker with a comment on top of the file like:

//spell-checker: ignore coreutil

Except for the compile error for char::from_utf8, all compile errors will soon go away with our new minimum rust version 1.51, so you can ignore those.

@sylvestre
Copy link
Contributor

Could you please fix the clippy warnings?


error: statics have by default a `'static` lifetime
  --> src\uu\tr\src\operation.rs:25:25
   |
25 |     pub static SPACES: &'static [char] = &[HT, LF, VT, FF, CR, SPACE];
   |                        -^^^^^^^------- help: consider removing `'static`: `&[char]`
   |
   = note: `-D clippy::redundant-static-lifetimes` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes

error: statics have by default a `'static` lifetime
  --> src\uu\tr\src\operation.rs:26:24
   |
26 |     pub static BLANK: &'static [char] = &[SPACE, HT];
   |                       -^^^^^^^------- help: consider removing `'static`: `&[char]`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes

error: this `else { if .. }` block can be collapsed
   --> src\uu\tr\src\operation.rs:433:16
    |
433 |           } else {
    |  ________________^
434 | |             if set1.is_empty() && set2.is_empty() {
435 | |                 Ok(TranslateOperationStandard {
436 | |                     translation_map: HashMap::new(),
...   |
440 | |             }
441 | |         }
    | |_________^
    |
    = note: `-D clippy::collapsible-else-if` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if
help: collapse nested if block
    |
433 |         } else if set1.is_empty() && set2.is_empty() {
434 |             Ok(TranslateOperationStandard {
435 |                 translation_map: HashMap::new(),
436 |             })
437 |         } else {
438 |             Err("when not truncating set1, string2 must be non-empty".to_string())
  ...

@hbina hbina marked this pull request as draft July 31, 2021 15:41
@hbina
Copy link
Contributor Author

hbina commented Jul 31, 2021

I am going to put it as draft again. Trying to pass GNU test.

@hbina hbina marked this pull request as ready for review August 1, 2021 02:44
@hbina
Copy link
Contributor Author

hbina commented Aug 1, 2021

I have since added all the GNU tests as rust tests --- Some of them are quite confusing to me so please take a look at it. I try to annotate the weird ones with #[ignore = "..."].

Aside from that, it passes all of them. So check if they are correct or not. I had to do some transformation to the GNU tests i.e. converting \n to \u{012} -- something like that.

@miDeb @tertsdiepraam

@sylvestre
Copy link
Contributor

a few failure errors
if you could have a look, it would be great
example of clippy

error: redundant closure
  --> src\uu\tr\src\convert.rs:16:22
   |
16 |                 .map(|u| char::from_u32(u))
   |                      ^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `char::from_u32`
   |
   = note: `-D clippy::redundant-closure` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

@sylvestre
Copy link
Contributor

I guess you saw also:


---- test_tr::check_against_gnu_tr_tests_bs_at_end stdout ----
current_directory_resolved: 
run: D:\a\coreutils\coreutils\target\x86_64-pc-windows-gnu\debug\coreutils.exe tr \ x
thread 'test_tr::check_against_gnu_tr_tests_bs_at_end' panicked at 'Command was expected to succeed.
stdout = 
 stderr = tr: missing operand after '\ x'
Try 'tr --help' for more information.
', tests\common\util.rs:167:13


failures:
    test_tr::check_against_gnu_tr_tests_bs_at_end

@hbina
Copy link
Contributor Author

hbina commented Aug 1, 2021

Oh in windows...

Edit: This is something windows-specific that I simply don't understand.

@hbina
Copy link
Contributor Author

hbina commented Aug 14, 2021

Some discussion on the current problem clap-rs/clap#2654

hbina added 12 commits August 14, 2021 19:36
Hopefully will be feature parity with GNU `tr`.

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Implemented a bit of new expansion module

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Implemented delete operation

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Partially implemented delete operation

Will go through translate next.

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Fix formatting...

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Implemented translation feature

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Ariffin <hanif.ariffin.4326@gmail.com>
Signed-off-by: Hanif Ariffin <hanif.ariffin.4326@gmail.com>
@hbina
Copy link
Contributor Author

hbina commented Nov 20, 2021

Something like this @sylvestre ?

@sylvestre
Copy link
Contributor

Do you know why the GNU test results aren't changing?
Changes from master: PASS +0 / FAIL +0 / ERROR +0 / SKIP +0

@hbina
Copy link
Contributor Author

hbina commented Nov 21, 2021

Prolly cause tr.pl test is failing.

2021-11-20T09:46:03.5959374Z FAIL: tests/misc/timeout-parameters.sh
2021-11-20T09:46:04.1405622Z FAIL: tests/misc/tr.pl
2021-11-20T09:46:04.2813337Z FAIL: tests/misc/tr-case-class.sh
2021-11-20T09:46:04.3913650Z FAIL: tests/misc/truncate-dangling-symlink.sh

Something wrong with the test environment?

2021-11-20T10:25:31.8416501Z FAIL tests/misc/tr.pl (exit status: 1)
2021-11-20T10:25:31.8416881Z 
2021-11-20T10:25:31.8417484Z FAIL: tests/misc/tr-case-class
2021-11-20T10:25:31.8417993Z ==============================
2021-11-20T10:25:31.8418269Z 
2021-11-20T10:25:31.8418826Z --- exp	2021-11-20 09:46:04.258060099 +0000
2021-11-20T10:25:31.8419590Z +++ out	2021-11-20 09:46:04.258060099 +0000
2021-11-20T10:25:31.8420113Z @@ -1,2 +0,0 @@
2021-11-20T10:25:31.8420877Z -tr: when translating with string1 longer than string2,
2021-11-20T10:25:31.8421803Z -the latter string must not end with a character class
2021-11-20T10:25:31.8423157Z ./tests/misc/tr-case-class.sh: line 65: warning: setlocale: LC_ALL: cannot change locale (en_US.ISO-8859-1): No such file or directory

@sylvestre
Copy link
Contributor

Hopefully, #2753 will improve that :)

@tertsdiepraam tertsdiepraam force-pushed the hbina-tr-reimplement-expansion branch 2 times, most recently from 106658e to 342a7ad Compare January 19, 2022 20:00
@tertsdiepraam tertsdiepraam force-pushed the hbina-tr-reimplement-expansion branch from 342a7ad to b51a6e8 Compare January 19, 2022 20:04
@tertsdiepraam
Copy link
Member

I've added some final changes and I think this is ready to be merged now!

@tertsdiepraam tertsdiepraam merged commit d2fe245 into uutils:main Jan 23, 2022
@rivy
Copy link
Member

rivy commented Jan 24, 2022

Yipes! That merged more than 15 sub-merge commits.
Let's, in future, crunch wild rides like this down a bit or rebase them.

@tertsdiepraam
Copy link
Member

Ah sorry! I didn't look at the history. Will keep that in mind in the future!

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.

tr does not support posix character class
5 participants