Skip to content

BF: Correctly ignore blank columns #5406

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

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

wader
Copy link
Contributor

@wader wader commented Feb 24, 2023

When skipping blank columns the index for the field name might end up incorrect cause a index of of range exception.

Example table:

| |a| |b|
|-------|
|1|2|3|4|

Would end up with:

Currently index 0 then index 2 will be remove in the already modified rangeCols list:

fieldNames = ["a", "b"]
rangeCols = [1,2]

With change:

fieldNames = ["a", "b"]
rangeCols = [0,1]

When skipping blank columns the index for the field name might end up incorrect
cause a index of of range exception.

Example table:
| |a| |b|
---------
|1|2|3|4|

Would end up with:

Currently index 0 then index 2 will be remove in the already modified rangeCols list:
fieldNames = ["a", "b"]
rangeCols = [1,2]

With change:
fieldNames = ["a", "b"]
rangeCols = [0,1]
@wader
Copy link
Contributor Author

wader commented Feb 24, 2023

Hi, i'm very new psychopy development. I was only able to test this in the builder, for some reason the local web runner does not find my generated files, returns 404, when running the dev version.

@peircej
Copy link
Member

peircej commented Mar 1, 2023

Hmm, interesting. So we're currently skipping the name without skipping the data. That does sound unwise, but I'm wondering if allowing empty headers at all is the problem. Like, what is the "expected behaviour" from a user if a column has no header?

@wader
Copy link
Contributor Author

wader commented Mar 1, 2023

Hi, i'm not a user myself, i helped someone that encountered this issue and it took quite a while to figure that it was a blank column. I'm think if there is no field name you can't refer to the data so should just be skipped? in this case some empty columns were used for comments/scratch data and whatnot so maybe it is nice to allow it?

@peircej peircej merged commit ad334c1 into psychopy:release Apr 5, 2023
@wader wader deleted the correct-skip-blank-columns branch April 5, 2023 08:17
@peircej
Copy link
Member

peircej commented Apr 5, 2023

I've pulled this for now as I don't have a better idea, but I still wonder if we should simply say "no" when we detect it

@wader
Copy link
Contributor Author

wader commented Apr 5, 2023

👍 yeah either way works i think, have no strong opinion. if it should show an error a very descriptive message would be good helping to find the affected column, maybe can mention the name of the non-broken column next to it etc?

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