refactor: sql lab command: separate concerns into different modules #16917
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ofekisr
force-pushed
the
refactor/sqllab3
branch
from
8e1100c
to
59f7d7b
Compare
ofekisr
force-pushed
the
refactor/sqllab3
branch
from
311b05f
to
d5788d5
Compare
amitmiran137
changed the title
ofekisr
force-pushed
the
refactor/sqllab3
branch
3 times, most recently
from
8957fb0
to
dcf24fb
Compare
ofekisr
force-pushed
the
refactor/sqllab3
branch
from
dcf24fb
to
98ca456
Compare
Codecov Report
@@ Coverage Diff @@
## master #16917 +/- ##
========================================
Coverage 77.03% 77.03%
========================================
Files 1021 1026 +5
Lines 54846 55005 +159
Branches 7481 7481
========================================
+ Hits 42250 42375 +125
- Misses 12349 12383 +34
Partials 247 247
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
amitmiran137
approved these changes
Oct 3, 2021
john-bodley
reviewed
Oct 7, 2021
| @@ -14,6 +14,7 @@ | |||
| # KIND, either express or implied. See the License for the | |||
| # specific language governing permissions and limitations | |||
| # under the License. | |||
| # pylint: disable=arguments-renamed | |||
There was a problem hiding this comment.
@ofekisr could we move these to the lines where the issue occurs? There was an effort in #16589 to remove top-level Pylint disable checks as it's i) unclear to the reader where these violations are occurring, and ii) potentially masks issues in future changes.
I realize that this wasn't clear and so adding it to the CONTRIBUTING.md in #17016.
There was a problem hiding this comment.
Hi @john-bodley as you notice this is the only change that was done to the file.
It was done bc all of a sudden pylint required it in the CI
The recent series of pylint rules changes that was done has awaken wiered behaviour and the CI and require us to work for the pylint instead of it working for us
linting all of sudden became a disturbance we need hop over on every PR
stevenuray
pushed a commit
to preset-io/superset
that referenced
this pull request
Oct 16, 2021
…odules (apache#16917)" This reverts commit 0d0c759.
mistercrunch
added a commit
to preset-io/superset
that referenced
this pull request
Oct 16, 2021
…odules (apache#16917)" This reverts commit 0d0c759.
sadpandajoe
added a commit
to preset-io/superset
that referenced
this pull request
Oct 18, 2021
…odules (apache#16917)" This reverts commit 0d0c759.
9 tasks
opus-42
pushed a commit
to opus-42/incubator-superset
that referenced
this pull request
Nov 14, 2021
…pache#16917) * chore move sql_execution_context to sqllab package * add new helper methods into base Dao * refactor separate get existing query concern from command * refactor separate query access validation concern * refactor separate get query's database concern from command * refactor separate get query rendering concern from command * refactor sqllab_execution_context * refactor separate creating payload for view * chore decouple command from superset app * fix pylint issues * fix failed tests * fix pylint issues * fix failed test * fix failed black * fix failed black * fix failed test
QAlexBall
pushed a commit
to QAlexBall/superset
that referenced
this pull request
Dec 28, 2021
…pache#16917) * chore move sql_execution_context to sqllab package * add new helper methods into base Dao * refactor separate get existing query concern from command * refactor separate query access validation concern * refactor separate get query's database concern from command * refactor separate get query rendering concern from command * refactor sqllab_execution_context * refactor separate creating payload for view * chore decouple command from superset app * fix pylint issues * fix failed tests * fix pylint issues * fix failed test * fix failed black * fix failed black * fix failed test
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
The sql_json view code in superset core view without any "clean code" standard and it does not adopt any software development principle.
This is the sixteenth PR in the sequence of future PRs (previous PR) try to solve it by refactoring the code.
The PR focus on separating sqlJsonCommand concerns into dependencies (interfaces)
actually, there are no logic changes so it implies on the current tests.