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

Fixes #183 #185

Merged
merged 8 commits into from
Oct 23, 2019
Merged

Fixes #183 #185

merged 8 commits into from
Oct 23, 2019

Conversation

nicklucius
Copy link
Contributor

Due to a change in an upstream data source, test code had broken. This fixes that code.

@nicklucius nicklucius self-assigned this Oct 23, 2019
@coveralls
Copy link

coveralls commented Oct 23, 2019

Coverage Status

Coverage decreased (-0.5%) to 83.784% when pulling 527a34e on issue183 into 8befd4f on master.

@geneorama
Copy link
Member

I'm more inclined to remove the column count tests unless we can comment why they're relevant.

I would also like to add some additional comments about what these tests are doing.

@nicklucius
Copy link
Contributor Author

@geneorama I think we will have a better answer about the column questions when we resolve #184. The test description is "Warn instead of fail if X-SODA2-* headers are missing" and we could add to that after resolving #184 as well.

If you want to remove the column checks while #184 is open I don't see an issue with that.

@geneorama
Copy link
Member

@nicklucius can you review please?

Made changes based on our conversation. I don't think the checks for data frame or column numbers were related to the original issue. The column number check was the part that was failing, so to me the cleanest way is to adjust the test.

@geneorama
Copy link
Member

Based on the verbal discussion, good idea to add the message relating to issue 118

@nicklucius
Copy link
Contributor Author

Looks good @geneorama

@nicklucius nicklucius merged commit 7791891 into master Oct 23, 2019
@geneorama
Copy link
Member

@nicklucius thanks. Glad we could get this knocked out.

@geneorama geneorama deleted the issue183 branch September 1, 2023 20:57
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.

3 participants