-
Notifications
You must be signed in to change notification settings - Fork 26
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
[CIVP-24324] FIX remove _get_headers
in read_civis_sql
#415
[CIVP-24324] FIX remove _get_headers
in read_civis_sql
#415
Conversation
- As it stands now, if an error is tripped in _get_headers, it silently fails. However, a simple fail can be invoked if the sql being checked is terminated with a semicolon. So a strip of the semicolon is added to cover a few more cases & get the optimization of that routine. Because the entire failure is labeled NOQA, tests aren't really possible. However, it was run with some queries to verify it doesn't blow up.
Sorry, I'm a bit confused. You want to remove semi-colons because it will throw the error or you want to remove the semi-colon because it will further suppress the error? |
So, it seems like the benefit of looking up the header can be done regardless of whether or not the query is terminated by a semicolon, no? |
ok, I get what you are saying. I have no opinion since i'm not familiar with this code. @mheilman do you have any thoughts? |
The I'm happy to provide context to things in the Python API client like this, but we do need to expand the bus factor and get other people involved in CR. |
Happy to just close this if that's the solution @jkulzick, unless you'd rather I modify it to remove _get_headers? |
@pytest.mark.read_civis | ||
@pytest.mark.skipif(not has_pandas, reason="pandas not installed") | ||
@mock.patch(api_import_str, return_value=civis_api_spec) | ||
def test_read_civis_pandas(self, *mocks): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These cassette-based tests wouldn't pass. Since we've been gradually removing/replacing these cassette tests with mock-based ones anyway (e.g, #328, #425), I'm doing the same here with the read_civis(_sql)
tests. (read_civis
is a thin wrapper of read_civis_sql
, and so below I'm explicitly testing read_civis_sql
only.)
@skomanduri Tagging you for review here, as we'd need someone with knowledge of the Platform code. In particular, is Kulzick's comment still accurate at this point given the current state of Platform? Thank you. |
_get_headers
in read_civis_sql
@jacksonlee-civis Yep, the comment is still accurate! This logic has been moved into Platform so it can be removed from the client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR removes the helper function
_get_headers
used bycivis.io.read_civis_sql
.Originally, @madopal discovered a bug in this helper function. John Kulzick then commented that this helper function should in fact be removed based on the current state of the internal Civis Platform code.
For reference, the following is @madopal 's original PR description:
optimization of that routine. Because the entire failure is labeled NOQA, tests aren't really possible. However, it was run with some queries to verify it doesn't blow up.