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

Issue 2325/indexing error #2485

Merged

Conversation

sebastianpiresmolin
Copy link
Contributor

Description

This PR fixes the off-by-one error in the importer error messages when the first row of the imported document is a header.

Screenshots

First row is header: Checked
Expected: 2
Actual: 2

image

First row is header: Unchecked
Expected: 1, 2
Actual: 1, 2

image

Changes

Import UseSheets from ../hooks/useSheets.ts to ../importMessage.ts so that the state of firstRowIsHeaders is usable.

Modify rowNumbers by adding a ternary that add +2 to index if firstRowIsHeaders or +1 if false.

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

Awesome work finding the code and fixing the bug in a simple way. I have reviewed the code and I have tried the preview build. It all works as expected! 💯

The only thing I would have done differently, is that I would not have made commits to try the hook, or at least not kept them around. They litter down the history which makes certain forensics tasks more difficult (i.e. when chasing down bugs).

If you feel the need to make a test commit like you've done here, you can either go back in history afterwards using git reset, or amend the commit with your new changes using git commit --amend, both of which would have allowed you to do the tests but then erase all history of it.

NOTE: You should never do either of the above with commits you have already pushed to a place that is shared with others, but I don't believe that was the case here.

In this case, what I will do is a "squash and merge", which means that I will squash together your three commits into a single commit that goes into the main history.

@richardolsson richardolsson merged commit 9f62ad7 into zetkin:main Jan 19, 2025
6 checks passed
@sebastianpiresmolin
Copy link
Contributor Author

Thanks @richardolsson for the feedback, I will definitely keep in mind to keep my commit history clean from unnecessary entries.

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