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

format docstrings #231

Closed
wants to merge 5 commits into from
Closed

format docstrings #231

wants to merge 5 commits into from

Conversation

bramtayl
Copy link
Contributor

@bramtayl bramtayl commented May 9, 2020

Ok, this should be mostly working now. Style and state should propagate to embedded code chunks. The only remaining issue is a problem with \\ (see the one new test broken) that I was hoping someone else would understand.

@bramtayl bramtayl changed the title Bramtayl format docstrings format docstrings May 9, 2020
@bramtayl
Copy link
Contributor Author

bramtayl commented May 9, 2020

Supercedes #225

@domluna
Copy link
Owner

domluna commented May 9, 2020

So the failed case has to do with string escaping and unescaping, which is one of the reasons the string is read directly from the file and not via .val field of the CSTParser node. I think the markdown parser is doing something similar.

It would be best to check with some other example docstrings where escaping/unescaping would alter the output.

@bramtayl
Copy link
Contributor Author

bramtayl commented May 9, 2020

I wonder if this is a bug in Markdown.plain? To be honest once it gets to 4 backslashes I get kinda lost

"""
`\\` \\\\
"""
test(x) = x
Markdown.plain(@doc test) # doesn't match text above

@bramtayl
Copy link
Contributor Author

bramtayl commented May 9, 2020

Also, I'm very confused about the format check failure...not sure how to fix the formatting when I'm changing the formatting simultaneously...

@domluna
Copy link
Owner

domluna commented May 9, 2020

You have to run format() on the JuliaFormatter.jl directory.

It's mostly there as a sanity check reminder to make sure the change hasn't caused unwanted changes in the files. Sort of like an integration test.

@bramtayl
Copy link
Contributor Author

bramtayl commented May 9, 2020

Ok got it. Well that seems to error, which is strange because the tests pass...I'll do some digging

@bramtayl
Copy link
Contributor Author

bramtayl commented May 9, 2020

Bad news: I definitely introduced a bug that wasn't caught by the tests. Good news: I found an extra test that reveals the bug.

@bramtayl
Copy link
Contributor Author

@domluna I can't figure out what's causing the bug, do you have any ideas?

@domluna
Copy link
Owner

domluna commented May 20, 2020

I'll take a look tomorrow. Probably related to docstrings being eaten somehow though

@bramtayl
Copy link
Contributor Author

bramtayl commented Jun 1, 2020

@domluna sorry to bother you again. Just looking forward to very pretty docstrings. Did you get a chance to look at this?

@domluna
Copy link
Owner

domluna commented Jun 1, 2020

feel free to bug me as much you like 🙂 . Could you rebase off master and push again?

Btw, regarding the rendering engine thing https://github.com/MichaelHatherly/CommonMark.jl looks promising but I haven't done a deep dive with it yet.

@mortenpi
Copy link

mortenpi commented Jun 3, 2020

Just a drive-by comment, which may or may not be useful: In Documenter we need to do something similar when we want to update the doctest outputs automatically. The approach we have taken is to just regex for the code blocks and then replace the content between the backticks. So we don't ever parse the Markdown, getting around the issue of the re-created Markdown not being identical to the original. And it seems to work quite reliably.

@bramtayl
Copy link
Contributor Author

bramtayl commented Jun 3, 2020

I think there's pluses and minuses of leaving the markdown part alone. On one hand, it guarantees round-trip-ability and is a lot simpler; on the other hand, there is potential to make markdown prettier too. Happy with either

@domluna
Copy link
Owner

domluna commented Jun 4, 2020

example of hooking into CommonMark to format julia code blocks MichaelHatherly/CommonMark.jl#4 (comment)

@bramtayl
Copy link
Contributor Author

bramtayl commented Jun 4, 2020

Ooh that's nice!

@bramtayl
Copy link
Contributor Author

bramtayl commented Jun 9, 2020

Rebased as #247

@bramtayl bramtayl closed this Jun 9, 2020
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.

3 participants