Skip to content

move sql linter to config instead of feat flag #6665

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Oct 4, 2025
Merged

move sql linter to config instead of feat flag #6665

merged 6 commits into from
Oct 4, 2025

Conversation

@Light2Dark
Copy link
Contributor

📝 Summary

Turns sql_linter on by default and moves it to config under Editor

CleanShot 2025-10-03 at 13 29 25

🔍 Description of Changes

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

@vercel
Copy link

vercel bot commented Oct 3, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
marimo-docs Ready Ready Preview Comment Oct 4, 2025 10:25am

@github-actions github-actions bot added the bash-focus Area to focus on during release bug bash label Oct 3, 2025
@github-actions
Copy link

github-actions bot commented Oct 3, 2025

Breaking changes detected in the OpenAPI specification!

@github-actions
Copy link

github-actions bot commented Oct 3, 2025

Breaking changes detected in the OpenAPI specification!

mscolnick
mscolnick reviewed Oct 3, 2025
View reviewed changes
mscolnick
mscolnick reviewed Oct 3, 2025
View reviewed changes
frontend/src/core/codemirror/language/languages/sql/sql.ts Outdated
return commentLines;
}

getExtension(): Extension[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

the last argument in getExtension is the Diagnostics, so we can use that to configure sql linter instead of putting it on this class (so it will reactively update the extensions)

mscolnick
mscolnick reviewed Oct 3, 2025
View reviewed changes
frontend/src/core/codemirror/language/languages/sql/sql.ts Outdated
constructor() {
try {
this.sqlLinterEnabled = getFeatureFlag("sql_linter");
this.sqlLinterEnabled = isSqlLinterEnabled();
Copy link
Contributor

Choose a reason for hiding this comment

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

for feature-flags it is fine if it requires a restart, but real config, it would be preferrable to have it enable when turned on without a refresh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, thanks! this should be ready and #6666 can be merged into this

@github-actions
Copy link

github-actions bot commented Oct 3, 2025

Breaking changes detected in the OpenAPI specification!

@Light2Dark Light2Dark marked this pull request as draft October 3, 2025 16:07
@github-actions
Copy link

github-actions bot commented Oct 3, 2025

Breaking changes detected in the OpenAPI specification!

mscolnick
mscolnick previously approved these changes Oct 3, 2025
View reviewed changes
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 3, 2025
@Light2Dark Light2Dark force-pushed the sham/move-sql-linter-to-config branch from 40fe492 to 4e5ce35 Compare October 4, 2025 10:12
@github-actions
Copy link

github-actions bot commented Oct 4, 2025

Breaking changes detected in the OpenAPI specification!

@Light2Dark Light2Dark force-pushed the sham/move-sql-linter-to-config branch from 4e5ce35 to 40fe492 Compare October 4, 2025 10:23
Light2Dark and others added 6 commits October 4, 2025 18:24
@Light2Dark
move sql linter to config instead of feat flag
9fd0548
@Light2Dark
better wording
58df330
@Light2Dark
update snapshot config
77969e5
@Light2Dark
move to use lsp config instead
d9e97e4
@Light2Dark
fix for test
1f695e0
@Light2Dark
release sql mode and update docs (#6666)
9812e79 
## 📝 Summary

<!--
Provide a concise summary of what this pull request is addressing.

If this PR fixes any issues, list them here by number (e.g., Fixes
#123).
-->

By default works for internal duckdb engine so is safe for release.

Some docs update too:
<img
src="https://github.com/user-attachments/assets/dcbd9916-0db0-4074-a147-727a797f0576"
width="600" />

## 🔍 Description of Changes

<!--
Detail the specific changes made in this pull request. Explain the
problem addressed and how it was resolved. If applicable, provide before
and after comparisons, screenshots, or any relevant details to help
reviewers understand the changes easily.
-->

## 📋 Checklist

- [x] I have read the [contributor
guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md).
- [ ] For large changes, or changes that affect the public API: this
change was discussed or approved through an issue, on
[Discord](https://marimo.io/discord?ref=pr), or the community
[discussions](https://github.com/marimo-team/marimo/discussions) (Please
provide a link if applicable).
- [x] I have added tests for the changes made.
- [x] I have run the code and verified that it works as expected.
@Light2Dark Light2Dark force-pushed the sham/move-sql-linter-to-config branch from 40fe492 to 9812e79 Compare October 4, 2025 10:24
@github-actions
Copy link

github-actions bot commented Oct 4, 2025

Breaking changes detected in the OpenAPI specification!

@Light2Dark Light2Dark marked this pull request as ready for review October 4, 2025 11:08
@mscolnick mscolnick merged commit 9b33221 into main Oct 4, 2025
44 of 47 checks passed
@mscolnick mscolnick deleted the sham/move-sql-linter-to-config branch October 4, 2025 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mscolnick mscolnick mscolnick approved these changes

@akshayka akshayka Awaiting requested review from akshayka akshayka is a code owner

@manzt manzt Awaiting requested review from manzt manzt is a code owner

Assignees

No one assigned

Labels

bash-focus Area to focus on during release bug bash documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Light2Dark @mscolnick