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

Feat run prettier on markdown #3093

Merged
merged 24 commits into from
Aug 24, 2019

Conversation

nathan-castlehow
Copy link
Contributor

@nathan-castlehow nathan-castlehow commented Jun 23, 2019

Description

Adds support for Prettier in editor to "Prettify" Markdown.
Hotkey option available in config. Prettier Config json also available in config.
Runs prettier over current markdown with configured prettier options.

prettier

Issue fixed

#3039

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • 🔘 Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • 🔘 I have attached a screenshot/video to visualize my change if possible

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Jun 24, 2019
@@ -28,6 +28,7 @@ import {generateInEditor, tocExistsInEditor} from 'browser/lib/markdown-toc-gene
import markdownlint from 'markdownlint'
import Jsonlint from 'jsonlint-mod'
import { DEFAULT_CONFIG } from '../main/lib/ConfigManager'
const prettier = require('prettier')
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace this with import statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #bde357f

Copy link
Member

Choose a reason for hiding this comment

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

Great!

"tabWidth": 4,
"semi": false,
"singleQuote": true,
"parser":"markdown"
Copy link
Member

Choose a reason for hiding this comment

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

Can you append this parser option at the end? Because this will never change so we should make it default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZeroX-DG do you mean as in don't have it in the options that show up in the config and then append the parser property before use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming i understood the comment correctly this should be addressed in #1d59d89

Copy link
Member

Choose a reason for hiding this comment

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

Yep that's exactly what I wanted, thank you!

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Jul 2, 2019
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Jul 7, 2019

One more small request, can you trigger the format table too? Currently it doesn't format the table.

@nathan-castlehow
Copy link
Contributor Author

nathan-castlehow commented Jul 7, 2019

One more small request, can you trigger the format table too? Currently it doesn't format the table.

@ZeroX-DG Do you have sample markdown and expected results? I assume we are meaning its not correctly formatting a markdown table?

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Jul 7, 2019

I never used prettier to format markdown before so I didn't realized that they have support for table. However, in this case the table is not formatted with the default config. Here's my input:

|asd| asdasd|
|---|--------|
|asd|asdasdasd|
|dasd|ASdasdasd|

it should produce this:

| asd  | asdasd    |
| ---- | --------- |
| asd  | asdasdasd |
| dasd | ASdasdasd |

@nathan-castlehow
Copy link
Contributor Author

nathan-castlehow commented Jul 8, 2019

@ZeroX-DG i just tried the table you had and it works ok for me?
My default config is
{
"trailingComma": "es5",
"tabWidth": 4,
"semi": false,
"singleQuote": true
}

What do you have as your default?

@nathan-castlehow
Copy link
Contributor Author

@ZeroX-DG have you had a chance to look at this?

@ZeroX-DG
Copy link
Member

@nathan-castlehow sorry for the late review. I found out about the reason behind this. If you have a line directly above the table then it will not format the table (or maybe format it wrongly).

asdasd
| asd  | asdasd|
| ---- | --------- |
| asd  | asdasdasd |
| dasd | ASdasdasd |

The code above will make the table to format wrongly. Is this a prettier bug or intended behavior?

@nathan-castlehow
Copy link
Contributor Author

nathan-castlehow commented Jul 20, 2019

@nathan-castlehow sorry for the late review. I found out about the reason behind this. If you have a line directly above the table then it will not format the table (or maybe format it wrongly).

asdasd
| asd  | asdasd|
| ---- | --------- |
| asd  | asdasdasd |
| dasd | ASdasdasd |

The code above will make the table to format wrongly. Is this a prettier bug or intended behavior?

Hi @ZeroX-DG, no worries.
Hmm great question.
When there is an empty line above the table (as below) it formats fine? Its either prettier intended behaviour or prettier bug i would say. Prettier uses remark-parse for dealing with the markdown.

asdasd

| asd  | asdasd|
 | ---- | --------- |
 | asd  | asdasdasd |
 | dasd | ASdasdasd |

@ZeroX-DG
Copy link
Member

Hmm...let's just say it's upstream issue for now. Can you fix the conflict so I can aprove it? @nathan-castlehow

@nathan-castlehow
Copy link
Contributor Author

Hmm...let's just say it's upstream issue for now. Can you fix the conflict so I can aprove it? @nathan-castlehow

@ZeroX-DG Should be all sorted now. Still understanding the ropes of git (use tfs at work) so apologies in advance if there are any issues. Let me know if you need me to fix anything.

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Aug 2, 2019
@Rokt33r Rokt33r added this to the v0.13.0 milestone Aug 24, 2019
@Rokt33r Rokt33r merged commit b9dd651 into BoostIO:master Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved 👍 Pull request has been approved by sufficient reviewers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants