Skip to content

fix: Detect negative numbers#193

Open
krlmlr wants to merge 1 commit intomechatroner:masterfrom
krlmlr:b-neg
Open

fix: Detect negative numbers#193
krlmlr wants to merge 1 commit intomechatroner:masterfrom
krlmlr:b-neg

Conversation

@krlmlr
Copy link
Copy Markdown
Contributor

@krlmlr krlmlr commented Jan 3, 2025

This allows an optional leading - .

Should we allow a leading + ? What about scientific notation?

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented Jan 21, 2025

@mechatroner: I tweaked the regex to account for the new TODO too.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented Jan 21, 2025

Should numbers like .3 or -.5 be supported too?

@mechatroner
Copy link
Copy Markdown
Owner

I appreciate you taking the time to fix this. The problem with the negative numbers IMO is that if the column is mixed-signed aligning them as numbers doesn't provide a lot of benefits since it doesn't allow for visual sorting that easily - one also has to take a look at the sign, so it is not much more convenient than just doing a standard length-based alignment. Also if we are handling negative signs the next step is to handle percent sign or dollar sign at the end so this will quickly get more complicated. Basically what I am saying is that current positive-only alignment already covers maybe 75% of use cases for numbers alignment and getting the remaining 25% is a matter of diminishing returns. I might revisit your proposal later, I will keep your pull request open for now.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented Jan 23, 2025

Thanks, I appreciate the explanation and the rationale. Not pushing for a change here, I can run from my fork 🙃

It is fairly common to have numeric data in CSV files, much less so percentages or values with a currency. It seems worth considering the case of negative numbers separately.

We could do pretty cool stuff with inlays here too:

  • left-align the leading minus, right-align the number
  • add thousand separators (zero-width or not) as inlays -- 12'345'678

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.

2 participants