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 indentation inside Code Examples #271

Merged
merged 4 commits into from
Oct 15, 2020
Merged

Fix indentation inside Code Examples #271

merged 4 commits into from
Oct 15, 2020

Conversation

dtinth
Copy link
Contributor

@dtinth dtinth commented Oct 8, 2020

Why:

In the Code Examples section, the YAML shown looks like, but is not, a valid GitHub Actions workflow file. The schedule: section is inside the on: where the docs shows it as a separate block.

What's being changed:

The indentation has been fixed to make the code example a valid workflow.

This helps the reader understand the context better that schedule: is not a standalone block, but is supposed to be in the on: section. It also prevents attentive readers from getting distracted by invalid YAML syntax in the code block while reading the guidelines.

Check off the following:

In the Code Examples section, the YAML shown looks like, but is not, a valid GitHub Actions workflow file.

The `schedule:` section is inside the `on:`.

This helps the reader understand the context better that `schedule:` is not a standalone block, but is supposed to be in the `on:` section.
@welcome
Copy link

welcome bot commented Oct 8, 2020

Thanks for opening this pull request! A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines.

@janiceilene
Copy link
Contributor

Thanks for opening a pull request @dtinth! I'll let the @github/docs-content-ecosystem team know this is ready for review 🌟

@janiceilene janiceilene added actions This issue or pull request should be reviewed by the docs actions team ecosystem This issue or pull request should be reviewed by the Docs Ecosystem team labels Oct 8, 2020
Copy link
Contributor

@rachmari rachmari left a comment

Choose a reason for hiding this comment

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

@dtinth thank you for updating these docs. 💚 🙌

I had one very minor request. Once you address that, we can merge this in! 🚀

```

- **Avoid:**

```
schedule:
- cron: "40 19 * * *"
schedule:
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we always start code blocks without any indentation, what do you think about reverting the indents on lines 128 and 129?

So the code example would be:

Use

on:
  schedule:
    - cron:  "40 19 * * *"

Avoid

schedule:
  - cron:  "40 19 * * *"

Copy link
Contributor Author

@dtinth dtinth Oct 10, 2020

Choose a reason for hiding this comment

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

@rachmari I think we just ran into a paradox 😂… As the code block in question is about “what to avoid” (which in my opinion should be exempt from the guideline).

  • The rule to “start code blocks without any indentation” also reinforces the guideline being addressed (to include the necessary context in code samples). Without the indentation the code example wouldn’t make sense; see the next point…

  • In workflow files, schedule is not a top-level property; it must be nested inside the on: block. YAML is whitespace-sensitive so removing the indentation will make the workflow file invalid. To show a valid part of a valid workflow file (which is what I am trying to convey) the indentation must be kept intact.

    ⬇️ This is not part of a valid workflow file (due to the missing indentation), so it might not be a good example...

    Avoid

    schedule:
      - cron:  "40 19 * * *"
    

    ⬇️ This is part of a valid workflow file but not following the guideline because the context is not included...

    Avoid

      schedule:
        - cron:  "40 19 * * *"
    

Let me know what you think. If you disagree with this I will make the changes as requested. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@dtinth, I see what you mean here about the paradox. 😂

The purpose of the style guideline is to let contributors know that when using code examples logical boundaries of the code example should be included. For example, if it were a javascript code sample it might be most relevant to include a whole function rather than a snippet inside a function. This specific example is referring to a GitHub Actions workflow file and specifically highlighting that you should include the top-level property on that schedule is related` to.

Sometimes we don't use the whole GitHub Actions workflow but just refer to steps. In that case, we ensure that we include the steps property at the top of the file.

for example:

steps:
 - name: My first step
   if: ${{ github.event_name == 'pull_request' && github.event.action == 'unassigned' }}
   run: echo This event is a pull request that had an assignee removed.

I suggested not indenting the Avoid example because it would conflate two style guidelines in one example. This specific example is referring to the guideline to show logic boundaries in code samples, not to avoid indenting the first line in a code sample. I think that would be a new style guideline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rachmari Thanks for taking time to explanation the rationale. 🙏

Having seen other parts of the docs, this now becomes clear that indentation in an action’s subsection is omitted. I have fixed the Avoid section in the code example as you suggested. 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

@dtinth

Having seen other parts of the docs, this now becomes clear that indentation in an action’s subsection is omitted.

I think the underlying issue is that we're not specific enough about what this guideline means in our style guide. We should improve that in the future.

@rachmari rachmari self-assigned this Oct 10, 2020
Copy link
Contributor

@rachmari rachmari left a comment

Choose a reason for hiding this comment

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

@dtinth this looks great. Thank you for your contribution and the chat! 🙌 I'll get this merged right away. 🚀 🤘

@rachmari rachmari merged commit 1ac145f into github:main Oct 15, 2020
@github-actions
Copy link
Contributor

Thanks very much for contributing! Your pull request has been merged 🎉 You should see your changes appear on the site in approximately 24 hours.

If you haven't already, you can add yourself to the list of contributors by creating a new comment in this PR using these instructions. Thanks again! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actions This issue or pull request should be reviewed by the docs actions team ecosystem This issue or pull request should be reviewed by the Docs Ecosystem team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants