Skip to content

SAS7BDAT parser: Fix page count #48004

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

Closed
wants to merge 1 commit into from
Closed

Conversation

jonashaag
Copy link
Contributor

@jonashaag jonashaag commented Aug 8, 2022

While page size is 32 bits (https://github.com/Roche/pyreadstat/blob/e0627c7cf406e4296a9af16036a563719fcba237/src/sas/readstat_sas.c#L245) the page count is word size (https://github.com/Roche/pyreadstat/blob/e0627c7cf406e4296a9af16036a563719fcba237/src/sas/readstat_sas.c#L270-L284).

Unfortunately I don't have a test file that I can share.

  • closes #xxxx (Replace xxxx with the Github issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@jonashaag jonashaag marked this pull request as ready for review August 8, 2022 12:19
@mroeschke mroeschke added the IO SAS SAS: read_sas label Aug 8, 2022
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Do you have an idea of how to generate a similar sas file that could trigger the bug in the past?

Could also use a whatsnew note for 1.5.

@jonashaag
Copy link
Contributor Author

jonashaag commented Aug 8, 2022

Hmm maybe we can modify an existing test file and read the page count attribute from the parser. Not really an end to end test but the best idea I have

@jonashaag
Copy link
Contributor Author

@mroeschke do you prefer the kind of test I suggested to no test?

@mroeschke
Copy link
Member

@mroeschke do you prefer the kind of test I suggested to no test?

Yeah your suggested test would be good too add, thanks.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Oct 3, 2022
@mroeschke
Copy link
Member

@jonashaag could you merge in main one more time? This looked okay

@jonashaag
Copy link
Contributor Author

See #48764 instead

@mroeschke
Copy link
Member

See #48764 instead

Cool that one looks better. Okay to close this then?

@jonashaag jonashaag closed this Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO SAS SAS: read_sas Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants