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

Format code blocks with prettier #3911

Merged
merged 1 commit into from
Jun 5, 2021

Conversation

eliasnorrby
Copy link
Contributor

@eliasnorrby eliasnorrby commented May 3, 2021

This PR aims to standardize the formatting of code blocks. Most importantly, it fixes a few cases of invalid indentation.

Most other changes are related to list indentation.

Changes

While reading the docs, I noticed a couple of issues and inconsistencies within code blocks. I asked myself whether this could be addressed in a somewhat automated manner, e.g. a script scanning files for code blocks and formatting them with something like prettier.

I then wrote said script. It grew a little out of proportion, but it was fun. (Here it is)

I realize this may be deemed trivial, but having written the script and all, I thought I'd open this PR anyway.

Notable changes:

Incorrect indentation of collections

Change: These issues have been fixed.

Some blocks have incorrect indentation, like this one in docs/tasks.md at line 171:

spec:
  steps:
  - name: step-with-limts
    resources:
      requests:
        memory: 1Gi
        cpu: 500m
      limits:
->       memory: 2Gi
        cpu: 800m

Inconsistent indentation of lists

Change: yaml lists are now indented

Some lists are 2 space indented:

- name: flags
  value:
    - "--set"
    - "arg1=foo"
    - "--randomflag"
    - "--someotherflag"

Whereas others are 0 space indented:

- name: flags
  value:
  - "--set"
  - "arg1=foo"
  - "--randomflag"
  - "--someotherflag"

Some blocks even mix the two. I'm not aware of a widespread preference for either. I certainly don't have one - I only wish for consistency. As it turns out, prettier prefers indenting lists. In building my formatting script, I would have liked to provide the option to enforce either, but prettier lacks a configuration option for this. I experimented with other means of formatting (i.e. ruamel.yaml) that prefer a 0 space indent, but they proved to cause too many other problems to be viable.

Indicating excluded lines

I chose to use # ... over ..., since that's still valid yaml. That is:

- name: flags
  # ...
  value:
  - "--randomflag"

over

- name: flags
  ...
  value:
  - "--randomflag"

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in or deleted (only if no user facing changes)

This commit aims to standardize the formatting of code blocks. Most
importantly, it fixes a few cases of invalid indentation.

Most other changes are related to list indentation. Both 0 space and 2
space indentation has been used throughout the docs, while prettier
prefers 2 space indentation.
@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 3, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 3, 2021

CLA Signed

The committers are authorized under a signed CLA.

@tekton-robot tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 3, 2021
@tekton-robot
Copy link
Collaborator

Hi @eliasnorrby. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 3, 2021
@eliasnorrby
Copy link
Contributor Author

/kind documentation

@tekton-robot tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label May 3, 2021
@eliasnorrby eliasnorrby marked this pull request as ready for review May 3, 2021 00:10
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2021
@eliasnorrby
Copy link
Contributor Author

/release-note-none

@tekton-robot
Copy link
Collaborator

@eliasnorrby: you can only set the release note label to release-note-none if the release-note block in the PR body text is empty or "none".

In response to this:

/release-note-none

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 3, 2021
@eliasnorrby
Copy link
Contributor Author

/release-note-none

@afrittoli
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 10, 2021
@eliasnorrby
Copy link
Contributor Author

/retest

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2021
@zhouhaibing089
Copy link
Contributor

zhouhaibing089 commented May 12, 2021

Is it going to be an issue if the released YAML specs has a different sequence indentation format? Most of Go-based tool (kubectl/kustomize/ytt), they don't indent sequence in YAML.

@eliasnorrby
Copy link
Contributor Author

@zhouhaibing089 I'm not sure I understand your question. The released YAML specs does indent sequences. This PR only touches documentation, and, if anything, I think the indentation used throughout the docs should be consistent with release.yaml.

Whether, on a larger scope, one should aim to use unindented sequences (like the tools you mention) sounds like another question. (But it's not like the kubernetes docs consistently use unindented sequences in their docs just because kubectl does.) 🤷‍♂️

@zhouhaibing089
Copy link
Contributor

zhouhaibing089 commented May 14, 2021

The released YAML specs does indent sequences.

Yeah, you're right. :) My assumption is too strong. 🤦

Hmm, I actually take my comment back(as I realized that latest YQ does the indentation for sequence) - also mentioned in go-yaml/yaml#661. Over time, the tools which rely on go-yaml might eventually update to v3, and then sequence indentation will become normal (Hmm, it's a bit unfortunate to me).

@eliasnorrby
Copy link
Contributor Author

Oh, didn't know about go-yaml v3, that's an interesting development! 👀

@pritidesai
Copy link
Member

/lgtm

thanks @eliasnorrby, it will be great to have some automated tool for such fixes and instructions on how to use it 🙃

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2021
@eliasnorrby
Copy link
Contributor Author

/test pull-tekton-alpha-integration-tests

@eliasnorrby
Copy link
Contributor Author

/test pull-tekton-pipeline-alpha-integration-tests
🤦

@pritidesai
Copy link
Member

/test pull-tekton-pipeline-alpha-integration-tests

@tekton-robot tekton-robot merged commit e76d413 into tektoncd:main Jun 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesnt merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants