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

Revert "Switch from numeral to numbro (#6344)" #6595

Merged
merged 2 commits into from
Nov 25, 2023

Conversation

eradman
Copy link
Collaborator

@eradman eradman commented Nov 14, 2023

This reverts commit f8934b8.

What type of PR is this?

  • Bug fix

Description

Using a format string of 0 does not round to the nearest integer in Numbro

Related Tickets & Documents

numbro/issues/745

This reverts commit f8934b8.

Using a format string of '0' does not round to the nearest integer in Numbro
BenjaminVanRyseghem/numbro#745
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Merging #6595 (d9c5478) into master (9df6f80) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6595   +/-   ##
=======================================
  Coverage   62.39%   62.39%           
=======================================
  Files         161      161           
  Lines       13179    13179           
  Branches     1796     1796           
=======================================
  Hits         8223     8223           
  Misses       4672     4672           
  Partials      284      284           

Copy link
Member

@justinclift justinclift left a comment

Choose a reason for hiding this comment

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

Well damn. Oh well, if you reckon this is the best approach for now, then I'm ok with it. 😄

@masayuki038
Copy link
Collaborator

Oh my god... But you realized it now. Nice catch !

@justinclift
Copy link
Member

Looking at the issue @eradman filed on the numbro repo, they might be able to suggest a workaround or fix the problem themselves.

So, we're probably best to leave this for a few days and see how that issue goes. If nothing substantial turns up by then, we can merge this to revert back to numeral.

@eradman
Copy link
Collaborator Author

eradman commented Nov 14, 2023

Agreed; we should wait and see if there is a workaround we can use.

@guidopetri guidopetri enabled auto-merge (squash) November 25, 2023 14:07
@guidopetri guidopetri merged commit 392b930 into getredash:master Nov 25, 2023
@masayuki038
Copy link
Collaborator

Hi @guidopetri @eradman Thanks for fixing this.
This PR was merged. So, I checked the behavior. However, it looks like it didn't change.

image
image

I checked the source code in redash/redash:preview. However, I didn't find any issues...

[masayuki@localhost redash]$ docker images | grep preview
redash/redash                 preview         e60210000a6f   2 days ago          1.81GB
[masayuki@localhost redash]$ docker ps | grep redash-server-1
f28d906942b2   redash/redash:preview                      "/app/bin/docker-ent…"   About an hour ago   Up About an hour             0.0.0.0:5678->5678/tcp, :::5678->5678/tcp, 0.0.0.0:5001->5000/tcp, :::5001->5000/tcp   redash-server-1
[masayuki@localhost redash]$ docker exec redash-server-1 cat ./viz-lib/src/lib/value-format.tsx | grep numeral.options.scalePercentBy100
numeral.options.scalePercentBy100 = false;

Am I doing something wrong?

@eradman eradman deleted the numbro branch November 28, 2023 14:42
@eradman
Copy link
Collaborator Author

eradman commented Nov 28, 2023

A build from top of tree (392b930) shows that the previous behavior was restored

value_percent

Perhaps there is a delay in updating the docker image?

@masayuki038
Copy link
Collaborator

@eradman Thanks for your confirmation!
It may be something wrong in my env. I will check it again. thank you.

@masayuki038
Copy link
Collaborator

Indeed, the image created with make compose_build gave the expected value...
I will check redash/redash:preview later.

@guidopetri
Copy link
Contributor

@masayuki038 it might be a caching issue, where docker didn't pull the latest image.

@masayuki038
Copy link
Collaborator

@guidopetri Thanks for your comment.
I ran docker rmi redash/redash:preview... I'll try it on another machine. thank you.

@masayuki038
Copy link
Collaborator

Today, I tried redash/redash:preview again and it is fine. It seemed to be a cache issue. thank you.

harveyrendell pushed a commit to pushpay/redash that referenced this pull request Jan 8, 2025
This reverts commit f8934b8.

Using a format string of '0' does not round to the nearest integer in Numbro
BenjaminVanRyseghem/numbro#745

Co-authored-by: github-actions <[email protected]>
Co-authored-by: Guido Petri <[email protected]>
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.

4 participants