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: use SQLWarning(String reason) constructor for correct DriverMana… #751

Merged
merged 2 commits into from Apr 4, 2017

Conversation

@jamesthomp
Copy link
Contributor

@jamesthomp jamesthomp commented Feb 15, 2017

…ger logging

Previously, the default constructor was used which only logs "SQLWarning: ", but no information about the warning.

@vlsi
Copy link
Member

@vlsi vlsi commented Feb 15, 2017

@jamesthomp , could you please add a test or two to validate the change?

@jamesthomp jamesthomp force-pushed the jamesthomp:master branch from 9254ab9 to 1adbcdf Feb 15, 2017
@codecov-io
Copy link

@codecov-io codecov-io commented Feb 15, 2017

Codecov Report

Merging #751 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #751      +/-   ##
============================================
+ Coverage     65.24%   65.27%   +0.02%     
- Complexity     3495     3496       +1     
============================================
  Files           164      164              
  Lines         15125    15123       -2     
  Branches       2450     2450              
============================================
+ Hits           9869     9871       +2     
+ Misses         4078     4075       -3     
+ Partials       1178     1177       -1

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 95a3f41...de57227. Read the comment docs.

@jamesthomp jamesthomp force-pushed the jamesthomp:master branch from 1adbcdf to eedf8fe Feb 15, 2017
@jamesthomp
Copy link
Contributor Author

@jamesthomp jamesthomp commented Feb 15, 2017

Thanks for your prompt reply, @vlsi.
I have added a test demonstrating that the DriverManager now logs the full message with this change.

Let me know if any other changes are needed.

@jamesthomp jamesthomp force-pushed the jamesthomp:master branch from eedf8fe to 57a1e5e Feb 15, 2017
@vlsi
Copy link
Member

@vlsi vlsi commented Feb 15, 2017

I have added a test demonstrating that the DriverManager now logs the full message with this change.

I thought of more like a end-to-end test. That is execute SQL with error and ensure the message is there.

As far as I can see, it is even better to use SQLWarning(String reason, String SQLState, int vendorCode) constructor like in PSQLException. Can you do that?

@jamesthomp jamesthomp force-pushed the jamesthomp:master branch from 57a1e5e to 4911d1a Feb 15, 2017
@jamesthomp
Copy link
Contributor Author

@jamesthomp jamesthomp commented Feb 15, 2017

I didn't know what a good value for the vendorCode would be, so I just switched it to the SQLWarning(String reason, String SQLState) constructor.

I agree that an ETE test would be nicer than hardcoding the test, though I'm actually not very familiar with this codebase so I'd need to poke around a bit if you would like me to add that kind of test. Alternatively, I'm happy to add you as a collaborator on my fork @vlsi if you would prefer to just make the addition yourself (or you can just recreate this branch on this repo and then add the ETE test). Let me know which of these three options works best for you.

@vlsi
Copy link
Member

@vlsi vlsi commented Feb 18, 2017

@jamesthomp , could you check something around

@Test
public void testWarningsAreCleared() throws SQLException {
Statement stmt = con.createStatement();
// Will generate a NOTICE: for primary key index creation
stmt.execute("CREATE TEMP TABLE unused (a int primary key)");
stmt.executeQuery("SELECT 1");
// Executing another query should clear the warning from the first one.
assertNull(stmt.getWarnings());
stmt.close();
}
, add a test that would issue raise notice and assert if warning gets printed as desired?

toString, getMessage, and getSQLState should be removed since proper super(...) should be enough.

@davecramer
Copy link
Member

@davecramer davecramer commented Mar 15, 2017

@jamesthomp wondering if you have time to address @vlsi questions?

@jamesthomp
Copy link
Contributor Author

@jamesthomp jamesthomp commented Mar 16, 2017

Sorry for the delay - I was meaning to take another look at this but didn't get around to it just yet.

…ger logging

Previously, the default constructor was used which only logs "SQLWarning: ", but no information about the warning.
@jamesthomp jamesthomp force-pushed the jamesthomp:master branch 3 times, most recently from d1c8ecd to 871d6c5 Mar 19, 2017
James Thompson
@jamesthomp jamesthomp force-pushed the jamesthomp:master branch from 871d6c5 to de57227 Mar 19, 2017
@jamesthomp
Copy link
Contributor Author

@jamesthomp jamesthomp commented Mar 19, 2017

@vlsi I've kept getMessage, as removing it causes the format to slightly change which makes one of the tests in CallableStmtTest fail.

@jamesthomp
Copy link
Contributor Author

@jamesthomp jamesthomp commented Apr 3, 2017

Hey @vlsi / @davecramer - just wanted to check in and see if there is anything else you need me to do on this?

@davecramer davecramer merged commit 74a426b into pgjdbc:master Apr 4, 2017
2 checks passed
2 checks passed
codecov/project 65.27% (+0.02%) compared to 95a3f41
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
davecramer added a commit to davecramer/pgjdbc that referenced this pull request Sep 19, 2017
pgjdbc#751)

* fix: use SQLWarning(String reason) constructor for correct DriverManager logging

Previously, the default constructor was used which only logs "SQLWarning: ", but no information about the warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.