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

Fix formatting problems #623

Merged
merged 18 commits into from Dec 8, 2021

Conversation

maltekliemann
Copy link

While reading the docs, I noticed some formatting problems and went down the rabbit hole:

  • Indentation of Rust code in code blocks is often broken (2 space indents or tabs for indents)
  • Markdown indentation isn't always correct, leading to broken enumerations and incorrectly indented text and code blocks
  • Inconsistent rust code style

I've also noticed that running prettier on some of the files significantly changes some code, like that following <Message expressions. Is this indented? I have not committed the changes proposed by the auto-formatter.

@sacha-l
Copy link

sacha-l commented Dec 3, 2021

@maltekliemann - Thanks for this! Curious: did you fun a formatting tool to do this? Need to run the PR locally to double check, but looks good to me.

@maltekliemann
Copy link
Author

@sacha-l No, unfortunately, this had to be done by hand (except for replacing tabs with spaces). For example, you can't correct the indentation of enumeration items with a formatter - how is the formatter supposed to know where the indentation starts/ends? :)

@sacha-l
Copy link

sacha-l commented Dec 3, 2021

Cool - Yeah ! Unfortunately there is no ideal way to do this and although prettier is sometimes useful, other times it completely messes things up 🥶 🤷🏻

Copy link

@sacha-l sacha-l left a comment

Choose a reason for hiding this comment

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

So much better this way, thanks again.

@maltekliemann
Copy link
Author

Glad I could help! 👍

@sacha-l
Copy link

sacha-l commented Dec 3, 2021

Just resolving merge conflicts.

@sacha-l
Copy link

sacha-l commented Dec 6, 2021

Just resolving merge conflicts.

Update: resolved most merge conflicts with the exception of /v3/tutorials/11-kitties-workshop/b-kitties-frontend/index.mdx . Can you have a look there and also address @nukemandan's request? Thanks.

@maltekliemann
Copy link
Author

maltekliemann commented Dec 6, 2021

This should fix the merge conflict. No idea why the CI fails now.

I'll get on replacing the spaces with tabs, and I'll use the option that's not broken by prettier (see above).

@maltekliemann
Copy link
Author

So what's going on with #666?

@nuke-web3
Copy link
Contributor

Check links unfortunately fails always due to docer CI auth keys not being available for non-org members 🙄 safe to ignore, but we should check locally for sure if you touched URLs anywhere.

yarn fmt:docs not used, prettier on each page modified here only.
@nuke-web3 nuke-web3 merged commit d802bb9 into substrate-developer-hub:develop Dec 8, 2021
Copy link
Contributor

@nuke-web3 nuke-web3 left a comment

Choose a reason for hiding this comment

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

Did a yarn fmt and hand cleaned all files to work at aminimum with this cc @sacha-l please remember to run this in your PRs moving forward ;)

I also ran yarn server:fresh and the yarn checklinks locally - found a broken img too.

@nuke-web3
Copy link
Contributor

ummmm wtf? I did not merge this. Now it's missing things. This automation @sacha-l for develop? should be turned off asap.

@nuke-web3
Copy link
Contributor

nuke-web3 commented Dec 8, 2021

https://github.com/substrate-developer-hub/substrate-docs/commits/develop somehow it's right here. 🤷
moving forward @maltekliemann I hope you keep contributing!! 😁

Please do target main and read our contributor guidelines to get a feel for the flow we are (attepting!!) to workout. #4

@maltekliemann
Copy link
Author

Thanks! 👍 I'll check out the guide and will try to keep up the contributions as I learn!

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.

None yet

3 participants