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

Use Remark beautifier for markdown files by default #2004

Merged
merged 8 commits into from Mar 2, 2018
Merged

Use Remark beautifier for markdown files by default #2004

merged 8 commits into from Mar 2, 2018

Conversation

kachkaev
Copy link
Contributor

@kachkaev kachkaev commented Jan 9, 2018

What does this implement/fix? Explain your changes.

Tidy Markdown was the first and only markdown beautifier in atom-beautify until Remark was added two years ago. As of January 2018, Remark seems to be a more promising solution for markdown according to the state of its community:

Tidy Markdown Remark
number of contributors 2 60
commits to master in 2017 11 50+
github stars 35 1235
downloads per months on npm 45K+ 660K+

I'd like to propose switching to Remark by default and thus serve atom-beautify users with something they are more likely to be familiar with.

Does this close any currently open issues?

This may positively affect #1990 because Tidy Markdown has some issues extracting code from code blocks. If Remark is made a default choice, we can simply disable code block beautification option for Tidy Markdown without any harm to most users.

Also, if the solution to #965 is found, the default markdown beautifier will better integrate with custom markdown preferences that users may have.

Checklist

Check all those that are applicable and complete.

  • Merged with latest master branch
  • Regenerate documentation with npm run docs
  • Add change details to CHANGELOG.md under "Next" section
  • Added examples for testing to examples/ directory
  • Travis CI passes (Mac support)
  • AppVeyor passes (Windows support)

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 9, 2018

Tests also need updating because of slightly different default behaviours of Remark and Tidy Markdown. But WDYT in general?

@stevenzeck
Copy link
Contributor

I'm not against this, but our remark version is really out of date (3 major versions behind). Between this, #2008 and #1990 it might be better to make it one PR since these files are all crossing one another. Also, it's preferable to use node.js for beautifiers if available so users don't have to install them separately. I would suggest looking for a global install of remark-cli first, then if there isn't one, fall back to the programmatic interface of Remark (which needs to be upgraded in atom-beautify).

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 26, 2018

@szeck87 so which PR would you recommend to leave as an umbrella for all these changes? I've been thinking that they deserve separate discussions and that only some suggestions will be accepted. I'll be happy to restructure my work if you recommend what to do.

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 26, 2018

Checking for remark-cli and falling back to a built-in remark via npm seems reasonable. Do we need any ticks in options or should I just search for node_modules/.bin/remark-cli, then remark-cli and finally require('remark-cli') on failure?

@stevenzeck
Copy link
Contributor

@Glavin001 what's your opinion on the PR structures for #2004, #2008, and #1990?

As far as checking for global versus options, I seem to remember working on something similar and tried to use requireg, but can't remember what became of it. Is the use of remark-cli just for use of a .remarkrc file? @wooorm does node version not support the use of those files?

@wooorm
Copy link

wooorm commented Jan 26, 2018

@szeck87 Sorry, I’m not entirely sure what your question is?

@stevenzeck
Copy link
Contributor

@wooorm yea sorry, let me start over. In your comment on #965, you suggested to use remark-cli instead of remark. What is the rationale there?

@wooorm
Copy link

wooorm commented Jan 26, 2018

  • remark-cli (based on unified-engine) supports file finding, config files, file ignoring, etc etc: it’s made for the file-system, and has a higher level interface.
  • remark (based on unified) is made for processing text, and to be used through code

@stevenzeck
Copy link
Contributor

On second thought @kachkaev I think these can all stay as separate PRs. But I would hold off on #1990, or at least the remark specific parts of it, until #2008 is finalized.

@stevenzeck
Copy link
Contributor

@kachkaev this PR needs a couple more things:

  • Can you update the link in remark.coffee to https://github.com/remarkjs/remark?
  • Update the examples under examples/nested-jsbeautifyrc/markdown/expected/test.md and examples/simple-jsbeautifyrc/markdown/expected/test.md and yaml-front-matter.md? The other two yaml named files are skipped so you can ignore them.

@kachkaev
Copy link
Contributor Author

@szeck87 done 👍

Copy link
Contributor

@stevenzeck stevenzeck left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Glavin001 Glavin001 self-assigned this Feb 6, 2018
---

test ing
test
ing
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like Remark simply did NOT beautify this file.
Original: https://raw.githubusercontent.com/kachkaev/atom-beautify/18e7f5ebad5f83e25a6a03e05ee15ada93329328/examples/simple-jsbeautifyrc/markdown/original/yaml-front-matter.md

---
layout:          default
title:        this is my title
description:     this is my description
---

 test
   ing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unfortunately remark does not format frontmatter and also does not collate the lines like in the example above. Shall we just remove this test for now?

Copy link
Owner

Choose a reason for hiding this comment

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

No, this would become a regression bug for Atom-Beautify. If our users assumed continued support for YAML front-matter formatting in their Markdown files, then switching the default will completely break their usage.

I'm leaning away from switching default to Remark unless we can beautify files as least as well as we had done previous with the default (TidyMarkdown). Next release should be a clear improvement, not a bug, to users.

Copy link
Contributor Author

@kachkaev kachkaev Feb 6, 2018

Choose a reason for hiding this comment

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

@wooorm do you think remark could address this anyhow with remark?

Copy link
Owner

Choose a reason for hiding this comment

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

@kachkaev could you describe the possible breaking changes in the CHANGELOG? If we are going to switch from Tidy Markdown to Remark and cause all of these issues, we need to make it clear the cause and solution for users who do like their formatting looking like before.

Copy link

Choose a reason for hiding this comment

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

What is “frontmatter”? Here you show YAML, but there also exists TOML and some JSON front matter format as well.

Yes, remark could support this, through plugins, something like remarkjs/ideas#3, and the linked issue contains some sample code, that should be made to format the YAML/TOML/JSON!

3. three
1. one
2. two
3. three
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like it worked. However, how can a user configure this indentation? It appears to be 3 spaces for - bullets and 2 spaces for numbered bullets, such that they align for both. This looks nice, however for our current users this will cause a lot of whitespace reformatting when they beautify their Markdown files. If we could configure the default to work like it did previous that would be ideal.

Original: https://raw.githubusercontent.com/kachkaev/atom-beautify/18e7f5ebad5f83e25a6a03e05ee15ada93329328/examples/simple-jsbeautifyrc/markdown/original/test.md

#            heading 1

- item
-  item
-   item

##     heading 2

1. one
2.  two
2.   three

Copy link
Owner

Choose a reason for hiding this comment

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

I think this option is listItemIndent or incrementListMarker: https://github.com/remarkjs/remark/tree/master/packages/remark-stringify#optionslistitemindent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I'm not sure we should be switching back to old indents. Yes, the existing users may experience a one-time problem after an update, but that's expected given that the formatter being used is now different. Anyone can switch back to Tidy Markdown and thus restore the original behaviour.

Four characters per intent is a very reasonable choice and sticking to this rule has more benefits in the long run. In any case, it's not the only breaking change that people might see.

Copy link
Owner

Choose a reason for hiding this comment

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

In light of the lack of YAML font-matter support and these indentation changes, we need to make sure the CHANGELOG describes all of the breaking changes made by switching to Remark. Also, I would like to have more examples/tests added for Markdown showcasing the value and expected result from switching to Remark.

@@ -18,7 +18,7 @@ module.exports = {
"md"
]

defaultBeautifier: "Tidy Markdown"
defaultBeautifier: "Remark"
Copy link
Owner

Choose a reason for hiding this comment

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

I think you make a good argument for defaulting to Remark 👍 . However, first need to address the changes in expected above.

@Glavin001
Copy link
Owner

Thank you for this Pull Request, @kachkaev! Great work so far. Close to merging 😄 .

@@ -3,7 +3,7 @@ Beautifier = require('./beautifier')

module.exports = class Remark extends Beautifier
name: "Remark"
link: "https://github.com/wooorm/remark"
link: "https://github.com/remarkjs/remark"
Copy link
Owner

Choose a reason for hiding this comment

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

@Glavin001 Glavin001 assigned kachkaev and unassigned Glavin001 Feb 6, 2018
@kachkaev
Copy link
Contributor Author

kachkaev commented Feb 6, 2018

Thanks for your feedback @Glavin001! I tried to address the questions you've raised and am happy to hear what you think again!

@kachkaev
Copy link
Contributor Author

kachkaev commented Feb 6, 2018

CI fails, but according to the logs, the problem is with examples/simple-jsbeautifyrc/tsx/expected/test.tsx, not markdown files 🤔

@kachkaev
Copy link
Contributor Author

kachkaev commented Feb 8, 2018

Hmm... After checking out markdown formatting with prettier in the last couple of days, I'm in doubt that we should switch from 2/3 space indents to a fixed indent of four. Prettier also uses remark, but it's only for parsing. The details on how they generate the output is in this feature PR: prettier/prettier#2943. There've been some changes since then, e.g. the use of * instead of - in unordered lists.

So what shall we do with list item spacing? 🤔

@Glavin001 Glavin001 merged commit b8fa552 into Glavin001:master Mar 2, 2018
@Glavin001
Copy link
Owner

Thank you for contributing and your great work, @kachkaev !!

@Glavin001 Glavin001 added this to the v0.32.0 milestone Mar 2, 2018
@Glavin001
Copy link
Owner

Published to v0.32.0

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

4 participants