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

Fixed width saver and loader don't round trip. (Columns expand with increasing number of spaces) #2255

Closed
daviewales opened this issue Jan 15, 2024 · 6 comments · Fixed by #2257

Comments

@daviewales
Copy link
Contributor

daviewales commented Jan 15, 2024

Small description

Open test.csv:

colours,counts
red,3
green,5
blue,8

Then save it as test.fixed:

colours   counts   
red       3        
green     5        
blue      8        

Each column is separated by three spaces.

Now, open the newly saved test.fixed, and save it as test.fixed.csv.
Upon inspection, you will see that the three spaces have been included as column data, rather than being discarded as a separator:

colours,counts
red       ,3     
green     ,5     
blue      ,8     

If you repeat this process, three spaces get appended to each column every time you repeat a round trip.

Expected result

I expect to be able to round trip from CSV to Fixed and back without extra spaces being added to the data.

Steps to reproduce with sample data and a .vd

test-fixed-saver.zip

Additional context

  • saul.pw/VisiData v3.0.1
  • Python 3.10.12
@daviewales daviewales added the bug label Jan 15, 2024
@daviewales
Copy link
Contributor Author

Another example to consider is saving a frequency sheet as .fixed. For example, suppose you have colours.csv:

colours
blue
blue
blue
green
green
green
green
orange
purple
red
red

The Frequency sheet looks like this:
image

If you save it as fixed, you get:

colours   count   percent   histogram                                
green           4     36.36 ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇   
blue            3     27.27 ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇             
red             2     18.18 ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇                      
orange          1      9.09 ▇▇▇▇▇▇▇▇▇                                
purple          1      9.09 ▇▇▇▇▇▇▇▇▇                                

Once again, there are three extra spaces inserted. However, because the count and percent are numeric, they are right-aligned. Due to the right-alignment, the three extra spaces are added on the left side of the numeric values, which causes them to not be aligned with the column headers. This breaks the fixed loader:

image

@anjakefala
Copy link
Collaborator

Most recent fixed width saver issue: #1849

@daviewales
Copy link
Contributor Author

daviewales commented Jan 15, 2024 via email

@daviewales
Copy link
Contributor Author

Hmm... I'm wondering if getMaxWidth is being used for conflicting purposes. Here, we want the max width of the data, but it looks like getMaxWidth is trying to get a max display width. Also, I'm wondering if this is where the extra spaces come from:

w = max(w, nlen)+2

@daviewales
Copy link
Contributor Author

daviewales commented Jan 15, 2024

I've added a draft PR with some experimentation. (#2257) It's a lot closer to what I expect. It separates columns by a single space. However, when reading, that space is still interpreted as data, so we still grow by a single space per round trip. I think the saver is good to go, but the loader needs some work to prevent it loading the column separation space as data. I also switched it to use the real max width, rather than the max width of the window. This will be slow, but I think it's OK, because it will only run when saving, and I think most people would want to save all the data without truncating it.

daviewales added a commit to daviewales/visidata that referenced this issue Jan 23, 2024
Don't add so many spaces between columns.
Use the max width of the data, rather than the max width of the window.
daviewales added a commit to daviewales/visidata that referenced this issue Jan 23, 2024
@daviewales
Copy link
Contributor Author

The PR above is ready for initial review.

anjakefala pushed a commit to daviewales/visidata that referenced this issue Feb 5, 2024
Don't add so many spaces between columns.
Use the max width of the data, rather than the max width of the window.
anjakefala pushed a commit to daviewales/visidata that referenced this issue Feb 5, 2024
anjakefala pushed a commit to daviewales/visidata that referenced this issue Feb 5, 2024
Don't add so many spaces between columns.
Use the max width of the data, rather than the max width of the window.
anjakefala pushed a commit to daviewales/visidata that referenced this issue Feb 5, 2024
anjakefala added a commit to daviewales/visidata that referenced this issue Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants