Fix clippy and tests fail due to simple problems #415

strange-dv · 2024-07-11T20:45:38Z

Closes #414

This PR fixes small issues that cause CI to fail due to failing tests and cargo clippy

Testing

Local testing using cargo test --workspace --no-fail-fast --all-features and cargo clippy --all --all-targets --all-features

strange-dv · 2024-07-13T02:47:01Z

tabled/src/lib.rs

-///     id: [u8; 4],
-///     #[tabled(format("{}.{}.{}.{}.{}", self.id[0], self.id[1], self.id[2], self.id[3], self.name)]
+///     id: u8,
+///     #[tabled(format("{}.{}", self.id, self.name))]


We do not support formatting using complex expressions like self.id[0] so I simplified this code a bit

That's actually one of my endless (not released patches) 😄
I did rewrite this support more then a few times.

I wonder what do you think in this regard.
Shall we support functions calls, array index etc?

#[tabled(format("{}.{}", some_function(self.id), self.name.one_more_field[100]))] id: u8,

Why I did rewrote it is cause it's impossible to support in inline enums 😞
And having 2 different approaches in 2 different places seeeems like not the right approach.

#[derive(Tabled)] enum Something { #[tabled(inline)] A { #[tabled(format("{}", self.field1, func(0, 1, self)))] // <- impossible to complex expressions with self field1: usize, }, B {} }

So anyway maybe you have some thoughts

What do you mean by rewriting? What do we have now if this is not possible to implement for inline structs? Is this the same functionality I assumed for the test fix? If so, I'm sure that supporting more advanced formatting functionality is a good feature, but obviously, there are many complications that can affect the decision of whether it's worth implementing or not.

If you have patches that extend this functionality beyond the scope of the changed test, what should this test look like to test as much of #[tabled(format()] as possible?

zhiburt · 2024-07-14T11:04:34Z

Hmmmm

I did not realized it since today.
The errors comes from unicode-rs/unicode-width#65

If on unicode-width="0.2.11" it all works but on newest its not.


failures:
    core::table_test::derived::table_emojie_multiline
    settings::render_settings::tab_isnot_handled_by_default_test
failures:
    util::string::tests::strip_test
failures:
    grid::format_configuration::tabs_arent_handled
    grid::render::doesnt_render_return_carige_0
    grid::render::doesnt_render_return_carige_1
    grid::render::hieroglyph_handling_2

zhiburt · 2024-07-14T11:05:57Z

I think we need put some thinking to how to resolve it.
Either to force 0.2.11 usage.
Or adjust used logic, accordingly.

Alexdelia · 2024-07-15T11:47:05Z

tabled/src/settings/width/wrap.rs

@@ -1431,7 +1441,7 @@ mod tests {
                "\u{1b}[37m(l\u{1b}[39m\u{1b}[37m\u{1b}[41marg\u{1b}[39m\u{1b}[49m\u{1b}[37mest \u{1b}[39m \n",
                "\u{1b}[37minteger \u{1b}[39m  \n",
                "\u{1b}[37mless than \u{1b}[39m\n",
-                "\u{1b}[37more equal \u{1b}[39m \n",
+                "\u{1b}[37mor equal \u{1b}[39m \n",


that's my fault, sorry folks

I've re-reviewed my previous fix typos MR to make sure I didn't make other mistakes and I've reverted these error

At the same time, I fix newly found typos

I hope you pardon my mistake and appreciate the MR

#416

Alexdelia · 2024-07-15T12:25:07Z

tabled_derive/src/parse/field_attr.rs

@@ -15,6 +15,7 @@ pub fn parse_field_attributes(
 }

 pub struct FieldAttr {
+    #[allow(dead_code)]


why choose to allow dead code?

I'd like to hear from @zhiburt on this.
I see this field was added in ba048db. I know it was a long time ago, but do you remember why it exists but never read or it's fine if I remove it?

As for those #[allow(dead_code)] that I left in the test code, I think the reason why they are there is more understandable - they are intended for fields that we never read in tests, but should be able to write to them

As for those #[allow(dead_code)] that I left in the test code, I think the reason why they are there is more understandable - they are intended for fields that we never read in tests, but should be able to write to them

correct;
though these test could be cleaner I guess

if I remove it?

Will be all right.

zhiburt · 2024-07-16T21:58:50Z

As far as I think about it nowadays
Could you make a 3 changes in Cargo.tomls (papergrid/Cargo.toml, tabled/Cargo.toml, testing_table/Cargo.toml)

-unicode-width = "0.1"
+unicode-width = "=0.1.11"

Once you do the failed tests must be fixed on its own.
And we could land it.

Alexdelia · 2024-07-17T13:26:12Z

tabled_derive/src/parse/type_attr.rs

@@ -14,6 +14,7 @@ pub fn parse_type_attributes(
 }

 pub struct TypeAttr {
+    #[allow(dead_code)]


shouldn't we also remove this dead code?

Damn, missed it, thanks

that's what reviews are for

…-cargo-clippy

zhiburt · 2024-07-18T01:06:44Z

OK seems good to me 💯

Thanks @Alexdelia for review
Thanks for commitment @strange-dv

Have a good week;
Take care

strange-dv added 6 commits July 11, 2024 23:44

Fix clippy

97da9e4

Make warnings go away

0a6644e

Make tests alive again (not fully functional)

efadefc

Return some tests to the useful state

88f7cf6

Fix formatting

a41eb54

Fix removed tests

df1270a

strange-dv commented Jul 13, 2024

View reviewed changes

strange-dv added 5 commits July 13, 2024 05:47

Fix formatting

7aece9f

Fix the flag test

2086f17

Fix a fix

ec8e9be

Fix for another fix, again

16cb6e8

Fix formatting

a378138

strange-dv marked this pull request as ready for review July 13, 2024 11:46

Make sure fix will fix the problem

a99b27b

Alexdelia reviewed Jul 15, 2024

View reviewed changes

fix unicode-width viersion and remevo unused fieild

257e58b

strange-dv requested review from Alexdelia and zhiburt July 17, 2024 13:23

Alexdelia reviewed Jul 17, 2024

View reviewed changes

remove one more unused fieild

8554cac

strange-dv requested a review from Alexdelia July 17, 2024 13:42

Alexdelia approved these changes Jul 17, 2024

View reviewed changes

strange-dv and others added 4 commits July 17, 2024 18:27

fix unparsed ident

8ef3a04

Fix cargo fmt

ff4c61d

Update MSRV

9c346d4

Remove MSRV requirment

5918067

zhiburt added 4 commits July 18, 2024 03:35

Revert unicode_dep && wrap update

35f78f6

cargo clippy

8fb764a

Merge branch 'master' of github.com:zhiburt/tabled into strangedv-fix…

c082733

…-cargo-clippy

fix

4c4f28f

zhiburt merged commit 7479803 into zhiburt:master Jul 18, 2024
79 checks passed

russellbanks mentioned this pull request Aug 13, 2024

Why is the unicode-width dep pinned to version 0.11.1? #423

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix clippy and tests fail due to simple problems #415

Fix clippy and tests fail due to simple problems #415

strange-dv commented Jul 11, 2024 •

edited

Loading

strange-dv Jul 13, 2024

zhiburt Jul 14, 2024 •

edited

Loading

strange-dv Jul 14, 2024

zhiburt commented Jul 14, 2024

zhiburt commented Jul 14, 2024

Alexdelia Jul 15, 2024 •

edited

Loading

Alexdelia Jul 15, 2024 •

edited

Loading

Alexdelia Jul 15, 2024

strange-dv Jul 15, 2024

zhiburt Jul 16, 2024

zhiburt commented Jul 16, 2024 •

edited

Loading

Alexdelia Jul 17, 2024

strange-dv Jul 17, 2024

Alexdelia Jul 17, 2024

zhiburt commented Jul 18, 2024

Fix clippy and tests fail due to simple problems #415

Fix clippy and tests fail due to simple problems #415

Conversation

strange-dv commented Jul 11, 2024 • edited Loading

Testing

Choose a reason for hiding this comment

zhiburt Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiburt commented Jul 14, 2024

zhiburt commented Jul 14, 2024

Alexdelia Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Alexdelia Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiburt commented Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiburt commented Jul 18, 2024

strange-dv commented Jul 11, 2024 •

edited

Loading

zhiburt Jul 14, 2024 •

edited

Loading

Alexdelia Jul 15, 2024 •

edited

Loading

Alexdelia Jul 15, 2024 •

edited

Loading

zhiburt commented Jul 16, 2024 •

edited

Loading