-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: Support parsing of SQL code blocks in Markdown files #598
base: main
Are you sure you want to change the base?
Conversation
Thank you for this! I'm mostly away from my computer this week but I'll try to review next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thank you!
Just a couple of nits, honestly. Really well done with this. I really, really appreciate the tests in particular. GHA is down right now, so the checks won't run, but assuming those pass I think we can get this merged really soon.
src/sqlfmt/api.py
Outdated
from mistletoe import Document | ||
from mistletoe.block_token import BlockToken, CodeFence | ||
from mistletoe.markdown_renderer import MarkdownRenderer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wrap this in try ... except ImportError
and either return the source or re-raise a SqlfmtError
to create a nicer error message for users who don't have the extra installed. I know right now we shouldn't hit that codepath due to the Mode property, but would be good to safeguard against that changing in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the try ... except
guard. I went ahead and subclassed SqlfmtError
since I didn't see any bare raise SqlfmtError
anywhere.
@michael-the1 Sorry I dropped the ball on this. I'd love to get this finalized. Looks like there are quite a few failing tests and lints. Do you want to work through those? Let me know if you get stuck. |
@tconbeer Life can get busy which I totally understand. The reason the tests are failing is because |
Ah, I see now that, in the Github workflow, dependencies are installed with I also see some other errors in the static analysis and the primer workflows. I'll take a look at those tomorrow. |
3e3c1df
to
35ace60
Compare
@tconbeer I think I fixed all issues, but with two notes:
|
Closes #593
There are two caveats that would be good to consider:
mistletoe
keeps all the important whitespace, but can toss out some unnecessary whitespace, such as trailing whitespace or double newlines. A good example is in theREADME.md
of this very repo:I think this is acceptable since it does not affect readability and should not affect output (i.e. when it's rendered to HTML).
sql
blocks are not formatted. I think this is acceptable. See example: