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

Add proseWrap: never option to prettier md parsing #22688

Closed
wants to merge 21 commits into from
Closed

Add proseWrap: never option to prettier md parsing #22688

wants to merge 21 commits into from

Conversation

kartikcho
Copy link
Contributor

Fixes #21707

@kartikcho kartikcho requested a review from a team as a code owner March 31, 2020 14:50
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

But lint check is failing. You should run prettier (i.e. yarn run format) and include changed files in this PR to make it pass.

@kartikcho
Copy link
Contributor Author

Oh, thanks ! I'll update it in a bit.

@kartikcho kartikcho requested review from a team as code owners April 10, 2020 13:53
@kartikcho
Copy link
Contributor Author

I ran yarn run format as you said but the linting tests still seem to be failing @vladar.
Now I used prettier --write . as the test suggests but I want to run all these tests locally first if I could to avoid another failing commit.
Is there a way to test it like CLI before pushing?
Also if you could help in any other way too, I would appreciate it. Thanks!

@kartikcho kartikcho requested a review from vladar April 12, 2020 16:05
@vladar
Copy link
Contributor

vladar commented Apr 16, 2020

@kartik918 Mind resolving conflicts? Also, you should probably merge master and run yarn in the root of the repo. We recently switched to Prettier 2.0 (#22520), so yarn is needed to update Prettier version locally.

As for linting in CI - it runs the following commands:

yarn lint:code
yarn lint:other
yarn typecheck
yarn check-repo-fields

Currently, it fails on yarn lint:other step. You should be able to run them locally if you need.

@kartikcho
Copy link
Contributor Author

Thanks for getting back on this, I did everything and have ran the tests locally successfully. They should be all good now.

@vladar vladar requested a review from tesseralis April 16, 2020 18:43
@vladar
Copy link
Contributor

vladar commented Apr 16, 2020

lint still fails 🤷

Did you run yarn in the repo root after merging master? You need this to update prettier to the latest version.

@kartikcho
Copy link
Contributor Author

Okay I did everything including yarn at the root to update dependencies. All 4 tests pass locally. I thought the tests were failing because of newer commits and it is still the same.

Screenshots:

yarn lint:code

test1

yarn lint:other

test2

Sorry for the wall of commits haha, please squash and merge if the tests go green some day.

@kartikcho
Copy link
Contributor Author

More Screenshots for other tests,

yarn check-repo-fields

test3

yarn typecheck

test4

This is a weird error since gatsby-core-utils should be visible to babel-preset-gatsby.
@vladar this might be causing it because everything else is ideally green, at least locally.

This reverts commit 386668a.
This reverts commit 31cc398.
@kartikcho kartikcho requested a review from a team as a code owner April 19, 2020 21:25
@kartikcho
Copy link
Contributor Author

Still getting this error after fixing referencing

Cannot find module 'gatsby-core-utils'.

@kartikcho
Copy link
Contributor Author

Fixed it, getting a new error. Isn't related to linting though "Error: Not implemented: navigation"

@pvdz
Copy link
Contributor

pvdz commented Apr 20, 2020

Considering the core change here is a one-liner, can I suggest you submit a new PR based on the current master? Just the two commits; prettier bump and the commit with all the prettier-changed changes. The change set and commits are getting a little unwieldy.

The change to gatsby/.prettierrc affects 226 files locally for me. Only .md files are affected.

I spot checked most changes and they look okay. Two things jumped out to me:

Code example that seems to get malformed:

diff --git a/docs/tutorial/writing-documentation-with-docz.md b/docs/tutorial/writing-documentation-with-docz.md
index 5546e1300..535ae15b4 100644
--- a/docs/tutorial/writing-documentation-with-docz.md
+++ b/docs/tutorial/writing-documentation-with-docz.md
@@ -123,10 +123,7 @@ name: Button
 menu: Components
 ---
 
-// highlight-start
-import { Playground, Props } from "docz"
-import { Button } from "./button"
-// highlight-end
+// highlight-start import { Playground, Props } from "docz" import { Button } from "./button" // highlight-end

That seems weird, a specific markdown format that's not taken into account by Prettier?

Another one that I'm not 100% sure about, but might be fine (there's a few of these), is

diff --git a/docs/docs/starters.md b/docs/docs/starters.md
index a9402f1ce..19e551b9e 100644
--- a/docs/docs/starters.md
+++ b/docs/docs/starters.md
@@ -63,7 +63,7 @@ gatsby new blog ./Code/my-local-starter
 Official starters are maintained by Gatsby.
 
 | Starter | Demo/Docs | Use case | Features |
-| -------------------------------------------------------------------------------------------- | ------------------------------------------------------------ | ------------------------------ | ---------------------------- |
+| --- | --- | --- | --- |
 | [gatsby-starter-default](https://github.com/gatsbyjs/gatsby-starter-default) | [Demo](https://gatsby-starter-default-demo.netlify.com/) | Appropriate for most use cases | General Gatsby site |
 | [gatsby-starter-blog](https://github.com/gatsbyjs/gatsby-starter-blog) | [Demo](https://gatsby-starter-blog-demo.netlify.com/) | Create a basic blog | Blog post pages and listings |
 | [gatsby-starter-hello-world](https://github.com/gatsbyjs/gatsby-starter-hello-world) | [Demo](https://gatsby-starter-hello-world-demo.netlify.com/) | Learn Gatsby | Gatsby bare essentials |

@kartikcho
Copy link
Contributor Author

@pvdz did you run tests after the formatting? If all the checks come back green, let me know and I'll open a new PR for it.
Also did you run yarn run format for prettier changes?

Thanks for reviewing this!

@pvdz
Copy link
Contributor

pvdz commented Apr 21, 2020

I did not run all tests (plus, the only files changed were .md files so I'm not sure any test is even affected by it, apart from linting). I ran yarn format to apply Prettier / eslint autofix.

Note that there's some internal discussion around using a different formatter for the docs so I'm going to hold off on this PR/change for a bit until that discussion resolves.

cc @tesseralis

@pvdz
Copy link
Contributor

pvdz commented Apr 21, 2020

Oh I see this issue was already referenced by that issue :p

@kartikcho
Copy link
Contributor Author

I'll still open a new PR with a cleaner commit history since it doesn't seem like remark-lint will be used anytime soon (also because this trivial PR took too long so I'd be happy if it got merged :). You can decide if you want to merge it.

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.

chore(docs): Add proseWrap: never option to prettier markdown parsing
3 participants