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 code fencing example in the style guidlines #12783

Merged
merged 2 commits into from
Mar 7, 2019

Conversation

DanyC97
Copy link
Contributor

@DanyC97 DanyC97 commented Feb 22, 2019

This PR should bring more clarity on the code fencing style guidelines.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2019
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. language/en Issues or PRs related to English language labels Feb 22, 2019
@DanyC97 DanyC97 force-pushed the align-on-shell-style branch from c348a8e to a444f1a Compare February 22, 2019 11:15
@netlify
Copy link

netlify bot commented Feb 22, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit c348a8e

https://deploy-preview-12783--kubernetes-io-master-staging.netlify.com

@DanyC97 DanyC97 force-pushed the align-on-shell-style branch from a444f1a to 0558760 Compare February 22, 2019 11:16
@netlify
Copy link

netlify bot commented Feb 22, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit a444f1a

https://deploy-preview-12783--kubernetes-io-master-staging.netlify.com

@DanyC97 DanyC97 force-pushed the align-on-shell-style branch from 0558760 to 96b59e7 Compare February 22, 2019 11:17
@netlify
Copy link

netlify bot commented Feb 22, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 8bbf749

https://deploy-preview-12783--kubernetes-io-master-staging.netlify.com

@DanyC97 DanyC97 force-pushed the align-on-shell-style branch from 96b59e7 to 9a0094f Compare February 22, 2019 11:18
@DanyC97
Copy link
Contributor Author

DanyC97 commented Feb 22, 2019

/assign @Bradamant3

@DanyC97
Copy link
Contributor Author

DanyC97 commented Feb 22, 2019

just a note to let people know that the shell code fencing was used instead of bash because of the below stats taken from en lang

~kubernetes/website/content/en/docs:align-on-shell-style*]# grep -R "\`\`\`bash" * | wc -l
     144

vs

kubernetes/website/content/en/docs:align-on-shell-style*]# grep -R "\`\`\`shell" * | wc -l
    1534

@@ -110,6 +110,12 @@ document, use the backtick (`).
<tr><th>Do</th><th>Don't</th></tr>
<tr><td>The <code>kubectl run</code> command creates a Deployment.</td><td>The "kubectl run" command creates a Deployment.</td></tr>
<tr><td>For declarative management, use <code>kubectl apply</code>.</td><td>For declarative management, use "kubectl apply".</td></tr>
<tr><td>For bash code use <code>shell</code>.</td><td>For bash use <code>bash</code> or <code>sh</code>.</td></tr>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems confusing. What should I use for code written for a POSIX shell? What about for code that specifically expects bash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about for code that specifically expects bash?

do you have an example for that ?

@makocchi-git
Copy link
Contributor

@DanyC97

I have a question about code fencing style.
What code style should I use for the command output?


example (using "^" instead of "`")

^^^shell
kubectl get pods
^^^

^^^???
NAME READY STATUS RESTARTS AGE
sample 1/1 Running 0 15m
^^^


shell or nothing?

@sftim
Copy link
Contributor

sftim commented Feb 22, 2019

@makocchi-git I asked a similar question as part of #12739
I think we don't have an answer yet.

@DanyC97
Copy link
Contributor Author

DanyC97 commented Feb 22, 2019

^^^shell
kubectl get pods
^^^

^^^???
NAME READY STATUS RESTARTS AGE
sample 1/1 Running 0 15m
^^^

@makocchi-git i could be missing s'thing here, why a command output can't be formatted like "```" ?
Now in the code i've seen people been using "```none" but imo that is not covered by hugo nor Linguist so i'm also confused.

Maybe the reviewers can chime in and clarify a bit but imo using backtick instead of "^^^" is better

@makocchi-git
Copy link
Contributor

@sftim Thank you. I watch and follow #12739.

@Bradamant3
Copy link
Contributor

/hold

Thanks for the PR and for all the related discussions in the related issue and PR.

But can you say a bit more about why we need this PR and especially the enforcement of it (which I see others offering in related PRs)? We don't have syntax highlighting in the docs output for snippets in different languages, so this isn't something that affects the visible usability of the docs, and it's hard to imagine enforcing compliance going forward.

We've tried pretty hard to keep the style guide simple, and compliance is already an ongoing point of discussion. So I'm concerned about adding complexity when the UX value isn't clear.

(In any case, this wouldn't be the right place for the proposed recommendations -- the section that's added to is about inline code formatting, not about fenced code blocks.)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2019
@DanyC97
Copy link
Contributor Author

DanyC97 commented Feb 22, 2019

@Bradamant3 thank you for taking the time to review it

But can you say a bit more about why we need this PR and especially the enforcement of it (which I see others offering in related PRs)?

For someone new to the code base it could be very confusing to pick the option:

  • is it "```bash" or any alias they feel it should be used - "```shell", "```sh" etc
  • is it "```go" or "```golang"
  • is it "```yaml" or "```yml"

You can argue that everyone can use whatever they want but i thought that since there is a style guide then whoever is reviewing the PR will be aware of it and will try to enforce it

We don't have syntax highlighting in the docs output for snippets in different languages, so this isn't something that affects the visible usability of the docs, and it's hard to imagine enforcing compliance going forward.

I agree it doesn't add value from user perspective (ie - business value if i can use this term) however from code/ developer perspective i personally believe there is some value - see above. Also i personally like to keep code clear/ clean

So I'm concerned about adding complexity when the UX value isn't clear.

okay, fair point

(In any case, this wouldn't be the right place for the proposed recommendations -- the section that's added to is about inline code formatting, not about fenced code blocks.)

i used this section because in the guide it says

For inline code in an HTML document, use the <code> tag. In a Markdown document, use the backtick (`)

There is no specific section for code fencing however i'm happy to change it - just let me know your preference

@steveperry-53
Copy link
Contributor

@DanyC97 Thank you for the PR. All, thanks for the discussion.

All things considered, I recommend keeping it simple: just use a triple backtick, and don't try to specify what kind of code it is. Based on my experience with this doc set, I don't think authors and reviewers will go to the trouble of making sure every code snippet has the proper language or shell label. Given that the labels have no effect on the rendered pages, I don't see the point in working toward making the docs adhere to this more specific form of labeling code snippets.

@sftim
Copy link
Contributor

sftim commented Feb 25, 2019

@steveperry-53 I'm not sure I've understood what you're saying here, specifically:

I don't think authors and reviewers will go to the trouble of making sure every code snippet has the proper language or shell label. Given that the labels have no effect on the rendered pages, I don't see the point in working toward making the docs adhere to this more specific form of labeling code snippets.

I see that https://kubernetes.io/docs/reference/kubectl/cheatsheet/#bash renders with highlighting, and that's something I find useful. It sounds like you might be saying that the highlighting there doesn't happen, or maybe that all code blocks are highlighted using the same rules whatever way they are labelled.
Can you clarify?

@DanyC97
Copy link
Contributor Author

DanyC97 commented Feb 25, 2019

in addition to what @sftim said, let me add my comments @steveperry-53

just use a triple backtick, and don't try to specify what kind of code it is.

if that is the case (although i do love coloring like @sftim already mentioned) fine but i'd like to see consistency across the board. i.e - strip all md files to adhere to this new rule if that is the case

I don't think authors and reviewers will go to the trouble of making sure every code snippet has the proper language or shell label.

happy to be the picky person in order to help with the compliance and keep the docs aligned with the guidelines

that the labels have no effect on the rendered pages

if that is the case then how come other pages do show some differences ?I'm aware of a page which use a custom shortcode but everyone else doesn't and nobody is using the built in hugo highlights shortcode

@steveperry-53
Copy link
Contributor

@DanyC97, Maybe I'm wrong about not getting highlighting differences. @Bradamant3, do you know whether the labels on code fences make any difference in the rendered snippets?

@Bradamant3
Copy link
Contributor

Thanks @zacharysarah for the clear and sensible summary.

@steveperry-53 I haven't seen other examples like the ones that sftim and Zach point to -- I think it's fair to say they're not common. I also thought I'd seen plenty of examples marked as shell that don't render like the example Zach pointed to, but quite possibly I'm mistaken. But that's really a side note.

@DanyC97
Copy link
Contributor Author

DanyC97 commented Feb 26, 2019

thank you all for taking the time to review and provide your input, much appreciated.
I'll re-do the PR and then i can move on with the rest.

@zacharysarah regarding

We need to avoid binary thinking about consistency. We don't need to require language-specific code fencing for all examples, nor should we remove all language-specific fencing for sheer consistency's sake

fair point, thank you 👍

@DanyC97 DanyC97 force-pushed the align-on-shell-style branch from 052816e to 6b886a0 Compare February 26, 2019 13:06
@DanyC97 DanyC97 changed the title Consistency around the code fencing style guidelines Add code fencing example in the style guidlines Feb 26, 2019
@DanyC97 DanyC97 force-pushed the align-on-shell-style branch from 6b886a0 to 826663a Compare February 26, 2019 13:09
@DanyC97
Copy link
Contributor Author

DanyC97 commented Feb 26, 2019

@zacharysarah pls help review

@DanyC97
Copy link
Contributor Author

DanyC97 commented Feb 26, 2019

@zacharysarah maybe for another PR i think we can (if you agree) standardize on the guidelines expressed here

And in that way we can kill 2 birds with 1 stone:

  • the output and how we do it today - see here which is bit too much if you ask me (nothing wrong with the PR though which follows the guidelines)
  • and consistency around sh/shell/bash etc

Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@DanyC97 Some more feedback.

content/en/docs/contribute/style/style-guide.md Outdated Show resolved Hide resolved
</table>

{{< note >}} Syntax highlighting is supported but optional {{< /note >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to end the sentence with a period. It's also helpful to specify what this is related to.

{{< note >}}
The website supports syntax highlighting for code samples, but specifying a language isn't required. For example, you can enable highlighting for Python samples by enclosing the sample like so: 
<code>```py
import foo
```</code>.
{{< /note >}}

Copy link
Contributor Author

@DanyC97 DanyC97 Feb 28, 2019

Choose a reason for hiding this comment

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

feedback taken, thanks.

just added the word ,is optional

@DanyC97 DanyC97 force-pushed the align-on-shell-style branch from 826663a to 587a2a5 Compare February 28, 2019 12:40
@DanyC97
Copy link
Contributor Author

DanyC97 commented Mar 4, 2019

@zacharysarah let me know if anything else is missing

@DanyC97
Copy link
Contributor Author

DanyC97 commented Mar 7, 2019

/sig docs

@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Mar 7, 2019
</table>

{{< note >}}
The website supports syntax highlighting for code samples, but specifying a language isn't required, is optional. For example, you can enable highlighting for Python samples by enclosing the sample like so:
Copy link
Contributor

@zacharysarah zacharysarah Mar 7, 2019

Choose a reason for hiding this comment

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

@DanyC97 One final nit. Let's use this wording instead:

The website supports syntax highlighting for code samples, but specifying a language is optional.

Otherwise LGTM 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@DanyC97 Be sure to remember the period at the end. 😉

@zacharysarah
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2019
@DanyC97 DanyC97 force-pushed the align-on-shell-style branch from 587a2a5 to 99c0bac Compare March 7, 2019 15:35
@zacharysarah
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zacharysarah

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit 8d4bd9c into kubernetes:master Mar 7, 2019
@DanyC97 DanyC97 deleted the align-on-shell-style branch March 7, 2019 23:16
krmayankk pushed a commit to krmayankk/kubernetes.github.io that referenced this pull request Mar 11, 2019
* Add code fencing example in the style guidlines

* Add a period
yagonobre pushed a commit to yagonobre/website that referenced this pull request Mar 14, 2019
* Add code fencing example in the style guidlines

* Add a period
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants