Skip to content

Conversation

@justinpark
Copy link
Member

SUMMARY

There's a regression caused by #23378 and #23908
This commit rollbacks the commit relates to the cleanSqlComments

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

CI

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API


const sqlContent = selectedText || sql || '';
const isDisabled = cleanSqlComments(sqlContent).length === 0;
const isDisabled =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe outside the scope of this PR, but I wonder if we should remove the isDisable logic as well. Regex's scare me.

Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #24009 (111eeae) into master (2a2f8a8) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head 111eeae differs from pull request most recent head 67ac8e8. Consider uploading reports for the commit 67ac8e8 to get more accurate results

@@            Coverage Diff             @@
##           master   #24009      +/-   ##
==========================================
- Coverage   68.21%   68.12%   -0.10%     
==========================================
  Files        1941     1941              
  Lines       75279    75274       -5     
  Branches     8166     8167       +1     
==========================================
- Hits        51350    51278      -72     
- Misses      21840    21907      +67     
  Partials     2089     2089              
Flag Coverage Δ
hive ?
javascript 54.51% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/actions/sqlLab.js 66.41% <ø> (-2.35%) ⬇️
...c/SqlLab/components/RunQueryActionButton/index.tsx 79.41% <100.00%> (ø)

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@justinpark justinpark merged commit 7a55625 into apache:master May 11, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 First shipped in 3.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.0.0 First shipped in 3.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants