Skip to content

Prettier Markdown files#3022

Closed
lipis wants to merge 14 commits intoprettier:masterfrom
lipis:prettier/md
Closed

Prettier Markdown files#3022
lipis wants to merge 14 commits intoprettier:masterfrom
lipis:prettier/md

Conversation

@lipis
Copy link
Copy Markdown
Member

@lipis lipis commented Oct 13, 2017

Some of the JS examples though must be ignored.

Let me know what would be the default print size and I'll edit the JS examples that needs to be reverted and not prettified.

@lipis lipis mentioned this pull request Oct 13, 2017
8 tasks

```js
ifBreak(";", " ")
ifBreak(";", " ");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, these are kind of expressions. Not statements, which is probably why there wasn't a semicolon here. Not the end of the world, I guess.

Copy the following config into your `.pre-commit-config.yaml` file:

```yaml

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this a bug, @ikatyang?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, the leading/trailing new line does nothing there.

http://spec.commonmark.org/0.25/#example-96

<summary><strong>Table of Contents</strong></summary>

- [Vim and Prettier integration](#vim-and-prettier-integration)
* [Neoformat](#neoformat)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is autogenerated so should be ignored.

vim-prettier executable resolution:

1. Tranverse parents and search for Prettier installation inside `node_modules`
2. Look for a global prettier installation
Copy link
Copy Markdown
Member

@azz azz Oct 13, 2017

Choose a reason for hiding this comment

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

I still think we should preserve this choice @ikatyang

Copy link
Copy Markdown
Member

@ikatyang ikatyang Oct 14, 2017

Choose a reason for hiding this comment

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

Should we add an option for it? or just print ordered numbers if the second number equals the first number +1?

EDIT: auto-detection should be better.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd say a good enough heuristic could be always use 1. iff items[1].number === "1.".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Opened #3024

Include this anywhere to force all parent groups to break. See `group` for more
info. Example:

```js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignore this block

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed.. maybe you can delete this comment so it won't confuse us


will output

```js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignore this block


and **not**

```js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignore this block


For example:

```js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignore this block


[link](https://github.com/prettier/prettier/compare/1.7.3...1.7.4)

* Force template literals to break after \` for styled-components (#2926 by duailibe)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ikatyang Should we pick * as unordered list instead?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should do some research for this too, just like the decision we made for strong/emphasis.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good idea. Done! #2943 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Opened #3025

Copy link
Copy Markdown
Member

@azz azz 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. As you mentioned there are a handful of places we want to ignore.

## Configure External Tool

https://blog.jetbrains.com/webstorm/2016/08/using-external-tools/
[https://blog.jetbrains.com/webstorm/2016/08/using-external-tools/](https://blog.jetbrains.com/webstorm/2016/08/using-external-tools/)
Copy link
Copy Markdown
Member

@ikatyang ikatyang Oct 14, 2017

Choose a reason for hiding this comment

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

This is a bug, we should print it with shortcut-style, i.e. <https://...>, opened #3030.

@lipis
Copy link
Copy Markdown
Member Author

lipis commented Oct 14, 2017

also I'm not sure how to incorporate markdown with the precommit hook.. but maybe we can do that after the official release..

@azz
Copy link
Copy Markdown
Member

azz commented Oct 22, 2017

@lipis there have been some changes to Markdown since you opened this PR. Did you want to re-run this with the current master?

@azz azz added the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Oct 22, 2017
@lipis
Copy link
Copy Markdown
Member Author

lipis commented Oct 22, 2017

yes I will do that once I'll get in front of my computer..

@azz azz removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Oct 23, 2017
@lipis
Copy link
Copy Markdown
Member Author

lipis commented Oct 23, 2017

should I fix anything else?

@lydell
Copy link
Copy Markdown
Member

lydell commented Oct 23, 2017

Yes, we need to figure out how to fix the "TOC should be up-to-date" test (which currently causes Travis to fail).

@azz
Copy link
Copy Markdown
Member

azz commented Oct 23, 2017

Would this work?

<!-- prettier-ignore -->
<details>
...

@lipis
Copy link
Copy Markdown
Member Author

lipis commented Oct 23, 2017

would be nice.. html support is coming soon as well as it looks like ;)

For example:

```js
matrix(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This block need to be <-- prettier-ignore -->d.

JSON:

```json
// .prettierrc
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't want this comment to move. Perhaps <!-- prettier-ignore --> this too.

editors/atom.md Outdated
@@ -1 +1,2 @@
See https://github.com/prettier/prettier-atom
See
[https://github.com/prettier/prettier-atom](https://github.com/prettier/prettier-atom)
Copy link
Copy Markdown
Member

@azz azz Oct 24, 2017

Choose a reason for hiding this comment

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

@ikatyang This one intentional? I thought it was fixed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems @lipis is still using the old prettier? The list style in this PR is still -, and so does this link.

@lipis
Copy link
Copy Markdown
Member Author

lipis commented Oct 24, 2017

Updated.. the following didn't work.. so I rerun the yarn toc afterwards..

<!-- prettier-ignore -->
<details>
...

README.md Outdated
For example:

```js
matrix(1, 0, 0, 0, 1, 0, 0, 0, 1);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The ignore comment needs to be above the fence otherwise it will show up in the rendered version.

```json
{
<!-- prettier-ignore -->
// .prettierrc
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here. Ignore needs to move above ```json

@lipis
Copy link
Copy Markdown
Member Author

lipis commented Oct 30, 2017

I think it's ready

@ikatyang
Copy link
Copy Markdown
Member

I think it'd be better to wait for the 1.8 release (or use current bin/prettier.js temporarily) to setup prettier --list-different <markdown-files> on travis in this PR to ensure our markdowns are formatted correctly.

@lipis
Copy link
Copy Markdown
Member Author

lipis commented Oct 31, 2017

Makes sense!

@lipis
Copy link
Copy Markdown
Member Author

lipis commented Nov 8, 2017

I think I will close that.. but the issue still remains.. we need to format all the md files with Prettier.. and use hooks for the future ones..

@lipis lipis closed this Nov 8, 2017
@lipis lipis deleted the prettier/md branch November 8, 2017 12:33
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants