[DBOPS-3] Updated SQL code style to match our current codebase #648
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Deploying contributing-docs with
|
| Latest commit: |
724450f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d1e45ddb.contributing-docs.pages.dev |
| Branch Preview URL: | https://sql-formatting-updates.contributing-docs.pages.dev |
- Standardized foreign key naming to include referenced table/column format - Added comprehensive SELECT statement formatting guidelines with table aliases - Clarified online index creation policy for self-hosted compatibility - Updated INSERT/UPDATE statement formatting examples with consistent parentheses alignment - Enhanced stored procedure and UDT formatting standards - Reorganized sections for better logical flow and readability
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
Excited to see this, as a lot of it has been tribal knowledge.
mkincaid-bw
changed the title
mkincaid-bw
changed the title
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
Minor thing and I think we're good.
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
My comments got wiped out I think.
|
@withinfocus All requested changes are in. |
| ## Best practices | ||
|
|
||
| 1. **Consistency**: Follow established patterns throughout the codebase | ||
| 2. **Readability**: Prioritize code readability and maintainability | ||
| 3. **Performance**: Consider index usage and query optimization | ||
| 4. **Security**: Use parameterized queries and proper data type validation | ||
| 5. **Modularity**: Break complex operations into smaller, reusable procedures | ||
| 6. **Standards**: Always use qualified object names with schema prefix | ||
| 7. **Versioning**: Use descriptive procedure names for different versions (e.g., `_V2` suffix) |
There was a problem hiding this comment.
This is a hallmark of AI writing, but I usually find it so general that it's not very useful. "Prioritize code readability and maintainability". Who's not doing that? :P
There was a problem hiding this comment.
While it's a bit boilerplate, an AI tool that reads this will be fueled to make more accurate changes.
| - `SELECT` keyword on its own line | ||
| - Column names indented (4 spaces) | ||
| - One column per line for multi-column selects | ||
| - Callout the specific table/alias for where a column is from when joining to other tables | ||
| - `FROM` keyword on separate line, aligned with `SELECT` | ||
| - `FROM` clause indented (4 spaces) | ||
| - Use aliases for table names when joining to other tables | ||
| - `JOIN` keywords on separate line, aligned with `FROM` | ||
| - Use full join specifications (`INNER JOIN` vs `JOIN`, `LEFT OUTER JOIN` vs `LEFT JOIN`, etc) | ||
| - `JOIN` clauses indented to align with table/column name(s) | ||
| - `WHERE` keyword on separate line, aligned with `FROM`/`JOIN` | ||
| - `WHERE` clause on separate lines, indented to align with table/column name(s) |
There was a problem hiding this comment.
I think this is excessively detailed and is unlikely to be referred to in practice. In my view the real solution to this is code linting and formatting tools which are sorely missing from server. The goal should be for devs to not think about formatting.
I would support any relevant team looking into that - dbops for MSSQL, Platform or Architecture for the codebase generally. (@withinfocus I have long given up any strong opinions about MegaLinter, I would much rather just have some solution here, if you have any interest in reviving that.)
There was a problem hiding this comment.
I have a fresher application of the linter at bitwarden/server#5938 and it would enforce at least some of this. Again though, AI agents will greatly benefit from its definition here. I do believe that we'll have essential human input to code reviews in perpetuity too.
| - **Trailing spaces**: Should be trimmed from the end of lines. Use `[ \t]+$` as a regex | ||
| find/replace |
There was a problem hiding this comment.
Similarly: this can/should be configured through IDE settings and formatters, not finding and replacing a regex string every time you edit a file. Even as an interim solution it's just not very useful advice.
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
I support documenting our practices, my feedback above is just about being intentional about what we document.
withinfocus
left a comment
withinfocus
left a comment
There was a problem hiding this comment.
Great work here, and please share the resulting link in the main engineering channel so others can read and benefit.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/DBOPS-3
📔 Objective
The goal for this PR is to clearly define the coding style, naming conventions, etc. for SQL code. This PR takes the existing doc and combines it with with a document generated by AI using on the existing SQL files in the code base as a baseline.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes