Pylint reports some missing functions in cqlshlib#4653
Pylint reports some missing functions in cqlshlib#4653jaibheem wants to merge 3 commits intoapache:trunkfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Addresses Pylint “missing member/function” reports in cqlshlib by adding explicit base-class method stubs and suppressing a known false-positive in the safer scanner implementation.
Changes:
- Disable Pylint
no-memberinsaferscanner.pyto avoid false positives from internalreAPIs. - Add abstract-style stubs (
dequote_*,escape_*) toCqlParsingRuleSetso Pylint can see expected methods.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pylib/cqlshlib/saferscanner.py | Adds a Pylint suppression for internal re member accesses. |
| pylib/cqlshlib/cqlhandling.py | Adds missing base-class method stubs used by completion/parsing logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # pylint: disable=no-member | ||
|
|
There was a problem hiding this comment.
The file-wide # pylint: disable=no-member suppresses all no-member checks in this module, which can hide real issues unrelated to the known false-positives from Python's internal re modules. Consider narrowing the suppression to the specific lines/blocks that access re.sre_parse/re.sre_compile/re._parser/re._compiler, or using a more targeted disable with an explanation comment.
pylib/cqlshlib/cqlhandling.py
Outdated
| raise NotImplementedError("Subclasses must implement dequote_value") | ||
|
|
||
| def escape_value(self, value): | ||
| raise NotImplementedError("Subclasses must implement dequote_value") | ||
|
|
||
| def escape_name(self, name): | ||
| raise NotImplementedError("Subclasses must implement dequote_value") |
There was a problem hiding this comment.
The NotImplementedError messages appear copy/pasted: dequote_name and escape_value both say subclasses must implement dequote_value, which is misleading when these methods are called. Update the messages to reference the correct method name (and consider keeping messages consistent across all four stubs).
| raise NotImplementedError("Subclasses must implement dequote_value") | |
| def escape_value(self, value): | |
| raise NotImplementedError("Subclasses must implement dequote_value") | |
| def escape_name(self, name): | |
| raise NotImplementedError("Subclasses must implement dequote_value") | |
| raise NotImplementedError("Subclasses must implement dequote_name") | |
| def escape_value(self, value): | |
| raise NotImplementedError("Subclasses must implement escape_value") | |
| def escape_name(self, name): | |
| raise NotImplementedError("Subclasses must implement escape_name") |
|
updated |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
I'm okay with this change but isn't there a decent argument for moving the current impls of the escape and dequote functions from the Cql3ParsingRuleSet subclass and back into the CqlParsingRuleSet superclass? The rules around escaping and quoting are much less likely to change between CQL versions (or at least it seems that way) so a default impl that covers most (all?) CQL versions would seem a reasonable candidate for the parent class. Rule set impls for individual CQL versions could then change if these rules around quoting did change in the future. Again, I'm not necessarily arguing that we have to make such a change... I'm fine with moving this forward as it stands. |
|
@absurdfarce @jaibheem sounds like a good idea to move the implementation up to the parent class. |
patch by Jai Bheemsen; reviewed by [Reviewers] for CASSANDRA-20400
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Follow up on review comments to move common quoting/escaping logic to the parent class for better reusability and cleaner inheritance. patch by Jai Bheemsen; reviewed by [Reviewers] for CASSANDRA-20400
jaibheem
force-pushed
the
CASSANDRA-20400
branch
from
b373626 to
25d0367
Compare
|
updated now, can you please take a look? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # pylint: disable=no-member | ||
|
|
||
| import re |
| def escape_name(name): | ||
| if name is None: | ||
| return 'NULL' | ||
| return "'%s'" % name.replace("'", "''") |
| self.assertEqual(CqlRuleSet.escape_value(None), "NULL") | ||
|
|
||
| def test_escape_name(self): | ||
| self.assertEqual(CqlRuleSet.escape_name("name'with'quote"), "'name''with''quote'") |
|
|
||
| from .basecase import BaseTestCase | ||
| from cqlshlib.cql3handling import CqlRuleSet | ||
|
|
Pylint reports some missing functions in cqlshlib
patch by Jai Bheemsen; reviewed by [Reviewers] for CASSANDRA-20400
The Cassandra Jira