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: losing all headers after a broken line #151

Merged
merged 5 commits into from
Sep 29, 2020

Conversation

aheissenberger
Copy link
Contributor

@aheissenberger aheissenberger commented Oct 10, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

when amount of columns of cell not equal to amount of headers the object returned is using indexed keys instead of the headers.
old version did only replace headers with index keys when there where more columns. When there where less columns the headers where still used.

Please Describe Your Changes

Problem #150

  • fix bug which replaced the global variable this.headers with indexed header when one line was broken
  • changed behavore to be identical for both use cases: less columns, more columns (this is a suggestion)

@codecov-io
Copy link

codecov-io commented Oct 10, 2019

Codecov Report

Merging #151 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   97.24%   97.26%   +0.01%     
==========================================
  Files           1        1              
  Lines         145      146       +1     
==========================================
+ Hits          141      142       +1     
  Misses          4        4
Impacted Files Coverage Δ
index.js 97.26% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 264e15a...240d075. Read the comment docs.

@aheissenberger
Copy link
Contributor Author

In an older version of this parser the logic for broken rows did not have any impact on the header mapping. What was the reason to change the behaviour to switch to indexed object if a line is broken?

@shellscape
Copy link
Collaborator

@aheissenberger that was likely a regression or untested side-effect when the indexed headers were added due to incomplete tests or poor coverage.

@aheissenberger
Copy link
Contributor Author

@shellscape using indexed props for columns with wrong amount of columns broke my code which was based on an older version of css-parser. I changed the code to be more consistent and the headers are now always set except for the case of more columns - here I will use the index for columns with no existing header.

@SimonSimCity
Copy link
Contributor

We've encountered the same problem and can confirm that this PR (as it is now) will fix the problem. Currently we're back on 2.3.0 until a version containing this fix is released.

@nathsimpson
Copy link

@mafintosh When can this be released?

@louisgv
Copy link

louisgv commented Dec 20, 2019

@shellscape @mafintosh would love to see this fix merged and published

@shellscape
Copy link
Collaborator

I won't have time to address any issues or PRs for this project until mid to late January, unfortunately.

@MalxMalx
Copy link

Hi, don't want to be seen as unpolite, but can you merge this PR? It's already February. I understand that you may have other important things on your table, but this problem affects my project, so I hope it can be fixed soon.

@SimonSimCity
Copy link
Contributor

@shellscape if you feel comfortable with the idea, I volunteer for helping you out with this project. I care very much about this project and want it to remain both healthy, in good shape and code-quality.

@shellscape
Copy link
Collaborator

@MalxMalx that's why it's open source. please use the fork this PR was based on.

@SimonSimCity not up to me unfortunately

Repository owner deleted a comment from MalxMalx Apr 1, 2020
@joshkay
Copy link

joshkay commented Jun 16, 2020

Also looking to have this merged. Updated this dependency and this bug makes it unusable for me currently. Downgrading to 2.3.0 where it was originally working for the time being. @shellscape @SimonSimCity would love to help you get this merged if possible

@stefan-jonasson
Copy link

I'm also waiting for this to be merged, anything I can do to help?

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.

10 participants