Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Stop using roaster for rendering markdown #559

Merged
merged 9 commits into from
Apr 25, 2019
Merged

Stop using roaster for rendering markdown #559

merged 9 commits into from
Apr 25, 2019

Conversation

rafeca
Copy link
Contributor

@rafeca rafeca commented Apr 23, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

In order to fix #206, the version of the marked package needed to be upgraded. Since this package depended on roaster, which is unmaintained and not even recommended to be used by its own creator, I've opted to replace it by directly calling marked and implementing in this package the missing functionality.

Alternate Designs

I could have forked roaster (in fact a really old fork of it already exists on the atom org: http://github.com/atom/roaster) and then upgrade marked from that fork, but I prefer to avoid having yet another fork and yet another package to maintain.

Benefits

Fixes #206

Possible Drawbacks

N/A

Applicable Issues

#206

@rafeca rafeca force-pushed the deprecate-roaster branch from 832e085 to 3245335 Compare April 23, 2019 18:37
@rafeca
Copy link
Contributor Author

rafeca commented Apr 23, 2019

In order to verify that the extension keeps working correctly, I've used the following markdown file:

https://gist.github.com/rafeca/7220f08c6d7d340533b3c323290ac507

Using standard style:

Screen Shot 2019-04-23 at 20 37 47

Using GitHub style:

Screen Shot 2019-04-23 at 20 38 08

@rafeca rafeca requested a review from a team April 24, 2019 13:37
Copy link
Contributor

@nathansobo nathansobo left a comment

Choose a reason for hiding this comment

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

This all seems reasonable. There's only one hole in the emoji handling I could think of, but I'm not sure it's worth fixing. If for some reason someone were to use an emoji string inside a URL, it would break rendering. Like this: [:finnadie:](https://foo.com/:finnadie:/). I think that's a valid URL, but I don't really think anybody would ever do something like this. So I say 🚢.

@rafeca
Copy link
Contributor Author

rafeca commented Apr 24, 2019

If for some reason someone were to use an emoji string inside a URL, it would break rendering. Like this: :finnadie:.

Good point! yeah this whole emoji thing is quite fragile (the logic in this PR mimics the one that we had before with roaster).

I wonder if we could get rid of the :colonemoji: handling for Markdown files (which also adds ~5.5MB to Atom due to all the emoji images being included in the app via the emoji-images packages), but I guess that this is such a distinctive GitHub feature that it's hard to justify removing it.

@rafeca rafeca merged commit 737c630 into master Apr 25, 2019
@rafeca rafeca deleted the deprecate-roaster branch April 25, 2019 08:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Last table cell isn't displayed if it doesn't have any data
2 participants