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

Docstrings highlighting with pygments #5462

Merged
merged 5 commits into from Oct 2, 2017

Conversation

nastasi-oq
Copy link
Contributor

@nastasi-oq nastasi-oq commented Sep 28, 2017

Currently:

  • tests are missing

Copy link
Collaborator

@carltongibson carltongibson left a comment

👀s good.

@nastasi-oq
Copy link
Contributor Author

nastasi-oq commented Sep 30, 2017

QUESTION @carltongibson: looking to adding test I found that apply_markdown(text): function (https://github.com/encode/django-rest-framework/blob/master/rest_framework/compat.py#L233)
has more or less the same behavior of render_markdown(markdown_text) function (https://github.com/encode/django-rest-framework/blob/master/rest_framework/templatetags/rest_framework.py#L70) but the first has tests and a better convertion in case markdown package is not present.
There is any particular reason because render_markdown isn't a simple wrapper of apply_markdown ?

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 30, 2017

@nastasi-oq I think “accident of history” is probably the simplest answer.

I’m happy to see a refactoring here but it might be better to handle it in two steps. This PR adding the new functionality, not worrying about duplication. Then another PR handling the refactoring. This makes each step easier to review, and complete. Sound ok to you?

@nastasi-oq
Copy link
Contributor Author

nastasi-oq commented Oct 1, 2017

@carltongibson IMHO there are 2 options:

  1. do what you suggest but in the reverse order
  2. do all in a single PR
    The reason is that extend the current test for apply_markdown is far simple and fast than develop a new one that in the second PR will be removed.
    If I have enough time before release I can do 1.

@nastasi-oq
Copy link
Contributor Author

nastasi-oq commented Oct 1, 2017

@carltongibson new PR opened to refactor render_markdown: #5469

@nastasi-oq nastasi-oq changed the title WIP: Docstrings highlighting with pygments Docstrings highlighting with pygments Oct 2, 2017
@nastasi-oq
Copy link
Contributor Author

nastasi-oq commented Oct 2, 2017

@carltongibson for me is final.

Copy link
Collaborator

@carltongibson carltongibson left a comment

This is lovely. Nice small footprint. Thanks for the work!

@carltongibson carltongibson merged commit 063534a into encode:master Oct 2, 2017
1 check passed
@nastasi-oq
Copy link
Contributor Author

nastasi-oq commented Oct 2, 2017

I'm glad to contribute to DRF.

@dmwyatt
Copy link
Contributor

dmwyatt commented Oct 15, 2017

@nastasi-oq Where did this @@ method of marking code blocks come from? Is that a standard somehwere or just something implemented here?

I ask because it's pretty widespread to mark code blocks in markdown with triple backticks. For example, that's how you mark code blocks here on github.

@nastasi-oq
Copy link
Contributor Author

nastasi-oq commented Oct 16, 2017

@dmwyatt just from the example that I found (and that is credited in the code), if you think to be better change it in another syntax I'm not against.

@dmwyatt
Copy link
Contributor

dmwyatt commented Oct 16, 2017

@carltongibson What do you think?

To be clear on what I'm talking about, this PR uses a format that is not used in any other markup system that I can find. Specifically, to mark a code block with this PR you do something like:

@@ json @@
['some json here']
@@

This '@@' markup is what I'm talking about.

Here on github and lots of other places you use triple backticks to markup code blocks. Of course, I don't think the Markdown "standard" (such as it is) has any method of marking code blocks, but I think triple backticks are widely-supported.

I propose code blocks be marked like:

```json
['some json here']
\```

(that extra backslash is just so I can fool githubs markdown parsing to let me put triple backticks inside a code block marked out by triple backticks)

@carltongibson
Copy link
Collaborator

carltongibson commented Oct 16, 2017

Yep. I’d happily review a PR making an adjustment here. I’m not particularly worried about the block markers, but the triple back ticks are widely used.

(In a way it would be nice to allow the markdown processor to be pluggable. I suspect that’s not going to be a hook we’d want to expose.)

@nastasi-oq
Copy link
Contributor Author

nastasi-oq commented Oct 19, 2017

@carltongibson always happy to make people happy: review it 😜 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants