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

Revert nextRowset changes #595

Merged
merged 3 commits into from
Nov 17, 2017
Merged

Revert nextRowset changes #595

merged 3 commits into from
Nov 17, 2017

Conversation

david-puglielli
Copy link
Contributor

@david-puglielli david-puglielli commented Nov 16, 2017

Reverting changes because several users have run into errors under the changes made to the way empty and null results are handled, and because the changes caused problems with multi-query statements. The original approach generally produced error messages only upon fetching, and next_result or nextRowset should simply move the cursor.

This change is Reviewable

@david-puglielli david-puglielli changed the title Revert next result changes, improve tests Revert nextRowset changes Nov 16, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 72.767% when pulling 633024c on david-puglielli:nextrowset-revert into 04f5034 on Microsoft:dev.

Copy link
Contributor

@yitam yitam left a comment

Choose a reason for hiding this comment

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

Please justify in this pull request why reverting to the previous way is necessary. Also, the tests can be further simplified / refactored to use much less code (too many repeated lines). It would also be easier to understand the test if you compare expected results / error messages within the test itself rather than relying on the expected output

@codecov-io
Copy link

codecov-io commented Nov 17, 2017

Codecov Report

Merging #595 into dev will decrease coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #595      +/-   ##
==========================================
- Coverage   74.47%   74.31%   -0.16%     
==========================================
  Files          50       50              
  Lines       14949    14900      -49     
==========================================
- Hits        11133    11073      -60     
- Misses       3816     3827      +11
Impacted Files Coverage Δ
...rojects/php/x86/php-7.0.25-src/ext/sqlsrv/conn.cpp 79.75% <0%> (-3.12%) ⬇️
.../php/x86/php-7.0.25-src/ext/pdo_sqlsrv/pdo_dbh.cpp 90.27% <0%> (-2.02%) ⬇️
...cts/php/x86/php-7.0.25-src/ext/sqlsrv/php_sqlsrv.h 60.71% <0%> (-0.47%) ⬇️
...rojects/php/x86/php-7.0.25-src/ext/sqlsrv/util.cpp 81.77% <0%> (-0.27%) ⬇️
...rojects/php/x86/php-7.1.11-src/ext/sqlsrv/stmt.cpp 87.67% <0%> (-0.27%) ⬇️
...rojects/php/x86/php-7.0.25-src/ext/sqlsrv/init.cpp 88.67% <0%> (-0.21%) ⬇️
.../php/x86/php-7.1.11-src/ext/pdo_sqlsrv/pdo_dbh.cpp 92.08% <0%> (-0.21%) ⬇️
...rojects/php/x86/php-7.0.25-src/ext/sqlsrv/stmt.cpp 87.82% <0%> (-0.17%) ⬇️
...x86/php-7.1.11-src/ext/sqlsrv/shared/core_stmt.cpp 80.63% <0%> (-0.12%) ⬇️
...x86/php-7.0.25-src/ext/sqlsrv/shared/core_stmt.cpp 78.76% <0%> (-0.12%) ⬇️
... and 7 more

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 04f5034...0088d61. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 72.767% when pulling b2e9621 on david-puglielli:nextrowset-revert into 04f5034 on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 72.767% when pulling 0088d61 on david-puglielli:nextrowset-revert into 04f5034 on Microsoft:dev.

@yitam
Copy link
Contributor

yitam commented Nov 22, 2017

#581

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.

5 participants