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

Move from github-markdown to github-markup #3919

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

russell
Copy link
Contributor

@russell russell commented Jan 25, 2021

They are very similar gems, but the former isn't supported on newer
versions of OSX. Basically you'll see an error like.

gh-markdown.c:56:29: error: implicitly declaring library function 'isspace' with type 'int (int)' [-Werror,-Wimplicit-function-declaration]
                while (i < lang->size && !isspace(lang->data[i]))
                                          ^
gh-markdown.c:56:29: note: include the header <ctype.h> or explicitly provide a declaration for 'isspace'

Since the github-markdown gem is deprecated, we need to change gems.

Risks

  • Low: anything that could go wrong will be covered in tests

@russell russell requested a review from grosser as a code owner January 25, 2021 15:46
@russell russell requested a review from a team January 25, 2021 15:46
They are very similar gems, but the former isn't supported on newer
versions of OSX.  Basically you'll see an error like.

```
gh-markdown.c:56:29: error: implicitly declaring library function 'isspace' with type 'int (int)' [-Werror,-Wimplicit-function-declaration]
                while (i < lang->size && !isspace(lang->data[i]))
                                          ^
gh-markdown.c:56:29: note: include the header <ctype.h> or explicitly provide a declaration for 'isspace'
```

Since the github-markdown gem is deprecated, we need to change gems.
@russell
Copy link
Contributor Author

russell commented Jan 26, 2021

@grosser i think we could actually trim this down a bit and only use the https://github.com/gjtorikian/commonmarker instead of the github wrapper, they turn on some options but we could do the same instead of using the gem?

https://github.com/github/markup/blob/cd01f9ec87c86ce5a7c70188a74ef40fc4669c5b/lib/github/markup/markdown.rb#L7-L10

Let me know what you'd prefer?

@grosser
Copy link
Contributor

grosser commented Jan 26, 2021

it's only used to display the risks, so if it renders a bit wrong that should not be an issue, just make sure it cannot be used for xss (like <script> being escaped etc)

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.

2 participants