Skip to content

csv: omit trailing empty header field#575

Merged
rewolff merged 1 commit into
traviscross:masterfrom
Komzpa:darafei/clean-csv-header
May 26, 2026
Merged

csv: omit trailing empty header field#575
rewolff merged 1 commit into
traviscross:masterfrom
Komzpa:darafei/clean-csv-header

Conversation

@Komzpa

@Komzpa Komzpa commented May 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Stop emitting a trailing delimiter at the end of the CSV header line.

The existing spacer field behavior is preserved: if the active field list contains a space field, CSV output keeps that column so existing column positions do not move. This only removes the unintended final empty header field.

Fixes #451.

Validation

  • ./bootstrap.sh
  • ./configure --without-gtk --without-jansson
  • make -j32
  • sudo -n ./mtr --report-cycles 1 --report --csv 127.0.0.1
  • Python csv check verifying:
    • header/data row counts match
    • the spacer column is still present as an empty CSV field
    • the last header/data field is not empty
  • git diff --check

@rewolff

rewolff commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Can you explain to me why I keep seeing the "15 files changed" and the same "patches" in the help-output, but still "no merge conflicts" ?

@Komzpa Komzpa force-pushed the darafei/clean-csv-header branch 2 times, most recently from 8da7362 to 8f30428 Compare May 25, 2026 18:39
@Komzpa

Komzpa commented May 25, 2026

Copy link
Copy Markdown
Contributor Author

You were right: this branch had accidentally picked up sibling stack commits. GitHub could still merge it cleanly, so it showed no merge conflict, but the PR comparison included those unrelated help-output/curses patches.

I rebuilt the branch on current master; it is now a single commit touching only man/mtr.8.in and ui/report.c, and the exact-head lint and compile-linux checks are green.

@rewolff

rewolff commented May 26, 2026

Copy link
Copy Markdown
Collaborator

FYI: The first time I thought "hmm". The second time "oh well" and only a bit later did I start "complaining".

Could you check the other open pull requests as well?

@rewolff

rewolff commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Now that the patch is "readable" for me....

What I think is happening is that you've eliminated the "empty fields" that you'd get when the format string contains a space.

I'm not sure I want to do that: Why would people want to use CSV? -- Because they want to process the data further.
So what happens when you change this behaviour? -- scripts will be looking in the wrong columns for their data.

People who didn't want the empty columns already had the option to not use spaces in the format string.

And when importing the CSV into a spreadsheet, having a way to create an empty column may be handy: Sometimes it provides a space for a formula that is based on the columns near it. (Sure, some users are fine with the "instructions" go to colmn 5, rightclick, select add column and proceed from there. I would not be: that can be automated: Add an empty column in the input file!)

So, you need to convince me that this is a good thing.

@Komzpa

Komzpa commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, checked.

There are no other currently open PRs from me in this repository besides #575. #575 is now rebuilt as one commit touching only man/mtr.8.in and ui/report.c, with lint and compile-linux green.

I also checked the same-day merged batch. Some of those did have the same stacked-branch review problem before merge. In particular, #639 included the stacked heads for #640-#645, and #636 included #638, so GitHub marked those sibling PRs merged together. #637, #580, and #576 also had noisy PR views earlier, although their final merge diffs into master ended up focused after prerequisites had landed.

Sorry for the confusing review surface; I’ll avoid sending stacked PR branches like that here.

@Komzpa

Komzpa commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

That makes sense as a compatibility concern.

The reason I sent this version is that in #451 you wrote that either removing the trailing comma or removing the empty header and 0 column would be acceptable:
#451 (comment)

This patch implements the second option, plus the trailing-comma cleanup. But I agree your point here is important: if users intentionally use the space field to reserve a CSV column, removing it changes column positions.

Would you prefer that I narrow this PR to only remove the trailing comma, and leave explicit spacer fields intact? That would fix the unintentional final empty field without changing the existing column layout contract.

@rewolff

rewolff commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Yeah, I think that's best. Just remove the empty column at the end. If someone imports it into a spreadsheet, current spreadsheets won't require you to "add a column" at the end: You can just start using more and more columns as you like, right?

Sorry for "changing my mind". Sometimes the better opinion comes a bit later. after I've already publicly stated a different one. Sorry! :-)

@Komzpa Komzpa force-pushed the darafei/clean-csv-header branch from 8f30428 to 89089f2 Compare May 26, 2026 10:39
@Komzpa Komzpa changed the title csv: omit spacer fields from output csv: omit trailing empty header field May 26, 2026
@rewolff rewolff merged commit ed63d3a into traviscross:master May 26, 2026
2 checks passed
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.

Fix header line generation for CSV data export

2 participants