Skip to content
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

[Bug]: status column values are misaligned in project list table output #962

Closed
1 task done
oddgrd opened this issue May 31, 2023 · 8 comments · Fixed by #1319
Closed
1 task done

[Bug]: status column values are misaligned in project list table output #962

oddgrd opened this issue May 31, 2023 · 8 comments · Fixed by #1319
Labels
Contribution Wanted The community is welcome to collaborate on this issue Good First Issue Good for newcomers S-Accepted This will be worked on T-Bug Something isn't working

Comments

@oddgrd
Copy link
Contributor

oddgrd commented May 31, 2023

What happened?

When running project list the values of the Status column are misaligned, this seems to be related to how we apply the color. If we remove the .<color> call from the project::State display impl the table will be correctly aligned, but we need the color in the display impl for other calls (like project::Response).

Screenshot of current output:
image

Version

v0.17.0

Which operating system(s) are you seeing the problem on?

Linux

Which CPU architectures are you seeing the problem on?

x86_64

Relevant log output

No response

Duplicate declaration

  • I have searched the issues and there are none like this.
@oddgrd oddgrd added T-Bug Something isn't working A-cargo-shuttle labels May 31, 2023
@jonaro00
Copy link
Member

jonaro00 commented Jun 1, 2023

Why does this not happen on deployment list?
image

@SyedFasiuddin
Copy link
Contributor

This difference might be causing this issue. I'm not sure.

pub fn get_color(&self) -> &str {

pub fn get_color(&self) -> Color {

I'll look into this.

@bayou-brogrammer
Copy link
Contributor

I did some digging into this and found the culprit. My first debugging attempt was to replace project.state with the string equivelent. I.E. Ready => "ready".

This printed the table with the correct format. Since the deployment list was experiencing no issues, I started comparing the setup of project list to deployment list. Turns out, that deployment state uses Strum::Display and project state use Strum::EnumString.

Switching out EnumString for Display showed the table in the correct formatting.

This issue now is we have a custom Display impl on Project Status. How do we want to proceed?

@bayou-brogrammer
Copy link
Contributor

image

@oddgrd
Copy link
Contributor Author

oddgrd commented Aug 7, 2023

Oh, sorry I missed this @lecoqjacob, thanks for figuring it out! Would you mind opening a PR for this so we can see the changes required? If you could include the screenshot of the output/take a new one, that would be great too.

@jonaro00
Copy link
Member

Deployment State uses .fg(state.get_color()) and colors the entire cell, while project State has more customization about which words are colored.

Shall we simplify the latter to only have one color for the entire string (for each state) associated with it? (make project State like deployment State)

@jonaro00 jonaro00 added S-Accepted This will be worked on Contribution Wanted The community is welcome to collaborate on this issue and removed A-common labels Aug 17, 2023
@jonaro00
Copy link
Member

We have now decided that the above comment is a good solution to this problem.
Anyone who wants, feel free to pick it up.
Let us know if more clarity is needed.

@jonaro00 jonaro00 added the Good First Issue Good for newcomers label Oct 10, 2023
@supleed2
Copy link
Contributor

supleed2 commented Oct 12, 2023

I'd like to try this issue.

So the goal is to replace the manual impl Display with the derive macro of Strum::Display yet somehow show the custom fields?

Looking at the macro, it uses std::fmt::Formatter::pad for the to_string attribute which doesn't allow formatting the fields, but maybe the serialize attribute might work?

Also, is the api.unstable.shuttle.rs endpoint still usable as a test, it would be good to get a json response with a range of different values?

Edit: I found a PR (Peternator7/strum#294) that would be useful, but it hasn't been merged, the maintainer seems to have been offline for a few weeks at least, and even once if it was marged, the version of strum used would need to be updated. What's the best option here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Wanted The community is welcome to collaborate on this issue Good First Issue Good for newcomers S-Accepted This will be worked on T-Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants