villebro commented on pull request #9673: URL: https://github.com/apache/incubator-superset/pull/9673#issuecomment-634155128
While I don't doubt this does what the PR description says, it would be great if we could transfer some understanding that you've gathered during research for this PR into the code/docs. Reviewing this is quite difficult in part because of the following: - Docstrings are very short or missing. Perhaps some examples could be added to the docstrings to explain what kind of tokens a typical SQL statement is decomposed into. - Documentation for `sqlparse` is also quite incomprehensive. - Variable names are not very descriptive. For example, `subtoken` might be more explanatory than `token2`. This might be overkill, but it would be great if we could add a test that ensures that `self._extract_from_token` for a certain test case isn't called more than x times. That would make sure this parsing logic stays performant in the future, too. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
