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

Fix csv columns potentially being numbered wrongly in the header when exporting #3690

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

iamllama
Copy link
Contributor

@iamllama iamllama commented Jan 5, 2025

Originally reported by @brishtibheja in https://forums.ankiweb.net/t/bug-guid-column-is-marked-as-deck-column/53924

With the way csv columns are currently numbered when exporting, a column selection that isn't a contiguous subsequence of guid, notetype, deck, tags* causes them to be numbered wrongly in the csv output.

By contiguous, e.g.
Fine: guid, notetype, deck or notetype, deck or deck etc.
Not fine: guid, deck or notetype, tags or guid, notetype, tags etc.

*the field(s) before tags don't affect this

1 + self
.deck_column()
.or_else(|| self.notetype_column())
.or_else(|| self.guid_column())
Copy link
Member

Choose a reason for hiding this comment

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

I think I'm missing something - why fall back on notetype/guid here when deck_column() does that already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for when the deck column isn't exported, in which case deck_column returns None and tags_column then has to fall back to the next earliest column(s)

Otherwise, the tags column would be 1 + 0 + the number of fields, potentially overlapping with the guid and notetype columns if they exist

This is the same for all the columns, they have to keep falling back to the next earliest non-None column

Copy link
Member

Choose a reason for hiding this comment

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

🤦 thanks :-)

@dae
Copy link
Member

dae commented Jan 9, 2025

@RumovZ if you have any preferences, please let us know

@RumovZ
Copy link
Collaborator

RumovZ commented Jan 12, 2025

Could have been handled better when I originally wrote it, so we're not reliant on getting the column order right in all the various places. But I think it's reasonable to just fix the current logic.

@dae dae merged commit afd7fca into ankitects:main Jan 13, 2025
1 check 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.

3 participants