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

YAML align indent sizes for docs readability #323

Merged
merged 5 commits into from
Apr 22, 2020

Conversation

nishigori
Copy link
Contributor

@nishigori nishigori commented Apr 19, 2020

This proposal has been aligned in a human readable.

I set the indentation size to the largest number of articles in the repository (dictionary: 2)

@nishigori nishigori force-pushed the align-yaml-indent-size branch 2 times, most recently from e8550c9 to cadb63b Compare April 19, 2020 17:55
@nishigori nishigori force-pushed the align-yaml-indent-size branch from cadb63b to 2520816 Compare April 19, 2020 17:57
@nishigori
Copy link
Contributor Author

I'm sorry, I just did sign contributor license agreement 😭

@igalic
Copy link
Collaborator

igalic commented Apr 19, 2020

does sphinx / read the docs actually display anything differently, or is this here just for people operating on the source?

@nishigori
Copy link
Contributor Author

Yes, sphinx / read the docs also makes indenting aligned.

For example, currently sphinx doc is not aligned indent-size
(following chef example is indent-size has 2, and 3)
https://cloudinit.readthedocs.io/en/latest/topics/modules.html#chef

Copy link
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

👍

@nishigori
Copy link
Contributor Author

@igalic thanks your review.

I was signed the contributor license agreement https://launchpad.net/~nishigori-tak

Please re-run github workflows for cla-validate
( I don't have writing privileges, so I can't run it 🙏 )

@OddBloke OddBloke self-assigned this Apr 20, 2020
@OddBloke
Copy link
Collaborator

Hi @nishigori, I see your signature in our internal tracking system. We just need you to add your name to https://github.com/canonical/cloud-init/blob/master/tools/.github-cla-signers (you should be able to do it as part of this PR), for our automated check to pass.

(I haven't reviewed this yet, FYI, just unblocking you on that particular part of the process. 👍)

@OddBloke
Copy link
Collaborator

OK, I'm going to have a look at this now. Thanks for the submission!

One idea that occurs to me is adding some sort of YAML indentation linting check to our CI, so that we will catch any inconsistencies before they're introduced in future. @nishigori, would you be interested in contributing such a check?

Copy link
Collaborator

@OddBloke OddBloke 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 taking this on, I think this will be a definite improvement to the docs. I've left some inline comments where I think you may have missed something, but overall this looks really good!

doc/examples/cloud-config-chef.txt Outdated Show resolved Hide resolved
doc/examples/cloud-config-disk-setup.txt Outdated Show resolved Hide resolved
permissions: '0644'
- content: |
# My new /etc/sysconfig/samba file
- encoding: b64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this (and all the following lines) be indented two extra spaces?

Copy link
Contributor Author

@nishigori nishigori Apr 22, 2020

Choose a reason for hiding this comment

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

It's not necessary, but I was aligned same like with other files

doc/examples/cloud-config-rsyslog.txt Outdated Show resolved Hide resolved
doc/examples/cloud-config.txt Outdated Show resolved Hide resolved
doc/rtd/topics/datasources/nocloud.rst Outdated Show resolved Hide resolved
doc/rtd/topics/tests.rst Outdated Show resolved Hide resolved
doc/rtd/topics/tests.rst Outdated Show resolved Hide resolved
doc/rtd/topics/tests.rst Outdated Show resolved Hide resolved
@nishigori
Copy link
Contributor Author

nishigori commented Apr 21, 2020

Hi, @OddBloke thanks your review.

One idea that occurs to me is adding some sort of YAML indentation linting check to our CI, so that we will catch any inconsistencies before they're introduced in future. @nishigori, would you be interested in contributing such a check?

Of course. Specifically, I'd like to turn yamllint or editorconfig in CI.

Your review points (mainly indentations in list) I'd like to fix it after running CI in this PR

@OddBloke
Copy link
Collaborator

OddBloke commented Apr 21, 2020 via email

@nishigori nishigori force-pushed the align-yaml-indent-size branch from b06ef42 to 476303f Compare April 22, 2020 02:05
@nishigori
Copy link
Contributor Author

Thanks, this PR is only fixed indent alignment (with your identified)

you can propose a linter as a follow-up PR. How does that sound?

That's right, it is for another time

@nishigori nishigori requested a review from OddBloke April 22, 2020 02:22
Copy link
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

LGTM now, thank you!

@OddBloke OddBloke merged commit 1b049e6 into canonical:master Apr 22, 2020
@nishigori nishigori deleted the align-yaml-indent-size branch April 23, 2020 03:24
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.

3 participants