Skip to content

CSV: provide column number and just the relevant cell on error#418

Open
Artoria2e5 wants to merge 2 commits intowikimediabrasil:mainfrom
Artoria2e5:patch-1
Open

CSV: provide column number and just the relevant cell on error#418
Artoria2e5 wants to merge 2 commits intowikimediabrasil:mainfrom
Artoria2e5:patch-1

Conversation

@Artoria2e5
Copy link

Fix #417


for index, cell in enumerate(row):
cell_value = cell.strip()
debuginfo = f"Column {index}: {cell_value}"
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 we should avoid english language in the code in a way that's not translatable. But I also don't know which syntax would look better. Just the cell_value maybe?

Copy link
Author

Choose a reason for hiding this comment

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

The col index is important additional info for locating the error, as cell_value can be repeated. I think "column" is some word everyone who deals with a database ends up learning and it actually beats many localized alternatives in terms of clarity (e.g. zh-CN has 行 列 for row, col but zh-TW has 列 行 for row, col). Translation of any debugging information is dubious to an extent.

For maximizing information content I would go for:

f"{index=},\n{header_value=}\n{cell_value=},"

]

commands.append(current_command)
commands.append((debuginfo, current_command))
Copy link
Member

Choose a reason for hiding this comment

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

Would there be a way to not have another variable here? Passing what it's needed in current_command

Copy link
Author

Choose a reason for hiding this comment

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

That would cause an unnecessary key in current_command, arguably a change in its datatype. I believe keeping the trace separate is better.

Updated debuginfo to include header information.
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.

enhancement: csv parser "raw" can perhaps just use the relevant cell

2 participants