Skip to content
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.

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

Comments, revisited #375

Merged
merged 12 commits into from
Jan 27, 2023
Merged

Comments, revisited #375

merged 12 commits into from
Jan 27, 2023

Conversation

tconbeer
Copy link
Owner

This PR ended up totally rethinking and rewriting comment handling. It was spurred by #348, but ended up doing more.

For the purposes of stability, we no longer push long inline comments to be standalone comments. This also helps
preserve the meaning of the position of the inline comments (this isn't always important, but sometimes it is).

This means that sometimes lines can exceed the max desired line length (including the inline comment).

  • feat: revisit comments, close Line order of comments and code is not preserved  #348
  • chore: improve unit testing and docs
  • fix: allow merging if lines end in trailing inline comment
  • fix: allow merging ahead of inline comments'
  • fix: allow merging of blank lines after inline comments
  • fix: keep inline comments inline
  • fix: strip whitespace when splitting comments onto multiple lines
  • fix: merge through comments if directly after standalone operator
  • chore: bump primer refs

@tconbeer tconbeer force-pushed the feat/#348/comments-revisited branch from 02fc97f to d98b7a7 Compare January 26, 2023 23:55
Copy link
Owner Author

@tconbeer tconbeer left a comment

Choose a reason for hiding this comment

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

Improve it

src/sqlfmt/merger.py Outdated Show resolved Hide resolved
src/sqlfmt/merger.py Outdated Show resolved Hide resolved
src/sqlfmt/splitter.py Show resolved Hide resolved
src/sqlfmt/splitter.py Outdated Show resolved Hide resolved
tests/data/unformatted/200_base_model.sql Outdated Show resolved Hide resolved
@tconbeer tconbeer merged commit 0f99ef6 into main Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Line order of comments and code is not preserved
1 participant