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: read blank lines and blank cells in CSV files #309

Merged
merged 4 commits into from
Jan 24, 2023

Conversation

ToddFincannonEI
Copy link
Collaborator

@ToddFincannonEI ToddFincannonEI commented Dec 16, 2022

Fixes #308

I enabled 'csv-parseflags to not skip empty cells and empty rows inreadCsv`. This fixes the test case in the issue. EPS is building OK with this change.

I don't know why these flags were disabled in the first place. Vensim formulas contain absolute cell references that will be incorrect if empty rows and cells are skipped.

The test:c-int model tests run OK. I wanted to add the test in the issue to directconst.mdl, but the recent h.csv file with non-numeric cell gives a fatal error in Vensim 9.2.1, so I could not produce the reference .dat file.

@chrispcampbell chrispcampbell changed the title read blank lines and blank cells in CSV files fix: read blank lines and blank cells in CSV files Jan 24, 2023
@chrispcampbell
Copy link
Contributor

@ToddFincannonEI:

I wanted to add the test in the issue to directconst.mdl, but the recent h.csv file with non-numeric cell gives a fatal error in Vensim 9.2.1, so I could not produce the reference .dat file.

I had left a comment in directconst.mdl regarding how to deal with the "invalid cell" test:

h[DimB, DimC] =
  GET DIRECT CONSTANTS(
    'data/h.csv',
    ',',
    'B2'
  )
  ~
  ~ This csv file has a cell that does not contain a number so that we can check that
    SDE does not throw an error in this case.  Note that Vensim will raise an error if
    a cell in the csv file does not contain a number, so to run this test in Vensim,
    temporarily change cell C3 to 0 in `h.csv`.

But I recognize that it was subtle and easy to overlook, so I moved that to its own model test (directconst_invalidcell) to avoid polluting the normal directconst test. After doing that, I took your test that was attached to the issue and folded it into the existing directconst test. So now we have test coverage for both issues.

The fix itself looks fine, so I'll merge this all shortly.

Copy link
Contributor

@chrispcampbell chrispcampbell left a comment

Choose a reason for hiding this comment

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

Looks good.

@chrispcampbell chrispcampbell merged commit 1a9fa37 into main Jan 24, 2023
@chrispcampbell chrispcampbell deleted the todd/308-csv-blank-line branch January 24, 2023 19:53
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.

CSV files with blank lines read incorrectly
2 participants