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

Actions: Indicate that .steps is an array of objects #1385

Merged
merged 7 commits into from
Dec 16, 2020

Conversation

Simran-B
Copy link
Contributor

@Simran-B Simran-B commented Nov 16, 2020

Why:

The headlines for job steps like jobs.<job_id>.steps.name imply the following YAML:

jobs:
  <job_id>:
    steps:
      name: Name

However, steps is actually an array of objects (note the hyphen):

jobs:
  <job_id>:
    steps:
      - name: Name

This should be indicated, e.g. like jobs.<job_id>.steps.*.name.

What's being changed:

Use .* to reflect that steps is an array. This is consistent with Object filters. It could be misinterpreted as shown below however:

jobs:
  <job_id>:
    steps:
      something:
        name: Name

Alternative ways to express it that come to mind:

  • steps[*].name (JSONPath style)
  • steps[].name (jq style)

Also fix angled bracket in headline outputs.<output_id.value>. already fixed by #1456

Check off the following:

@janiceilene
Copy link
Contributor

@Simran-B Thanks so much for opening a PR! I'll get this triaged for review 🌟

@janiceilene janiceilene added actions This issue or pull request should be reviewed by the docs actions team content This issue or pull request belongs to the Docs Content team ecosystem This issue or pull request should be reviewed by the Docs Ecosystem team labels Nov 17, 2020
@github-actions
Copy link
Contributor

This PR is stale because it has been open 7 days with no activity and will be automatically closed in 3 days. To keep this PR open, update the PR by adding a comment or pushing a commit.

@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Nov 25, 2020
@janiceilene janiceilene removed the stale There is no recent activity on this issue or pull request label Nov 30, 2020
@janiceilene
Copy link
Contributor

Thanks for your patience @Simran-B! Our small team is working our way through reviewing all of the amazing contributions ✨

@Simran-B
Copy link
Contributor Author

Simran-B commented Dec 1, 2020

No pressure, I'm in a very similar position myself, so I can totally relate.

BTW. I edited my initial post: jobs.<job_id>.steps.*.name could be misinterpreted and I'm actually in favor of jq's syntax jobs.<job_id>.steps[].name after some more thought.

@shati-patel
Copy link
Contributor

shati-patel commented Dec 2, 2020

(Just for information: I updated this PR from main to fix merge conflicts that came from #1389. I'll let someone from the Ecosystem docs team take a look at the actual changes, since they're more familiar with these topics!)

@github-actions
Copy link
Contributor

This PR is stale because it has been open 7 days with no activity and will be automatically closed in 3 days. To keep this PR open, update the PR by adding a comment or pushing a commit.

@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Dec 10, 2020
@janiceilene janiceilene removed the stale There is no recent activity on this issue or pull request label Dec 10, 2020
@lucascosti lucascosti self-assigned this Dec 14, 2020
@lucascosti
Copy link
Contributor

Thanks a lot for this PR, @Simran-B! Sorry it's taken us a while to respond.

IIUC, there's no standard specification for referring to a path for a yaml key (please correct me if I'm wrong! 🙂 )

I edited my initial post: jobs.<job_id>.steps.*.name could be misinterpreted

🤔 Yep, I agree with this. * could mean any item under steps, not specifically an array.

I think I would prefer using yq's syntax for referring to an array path: jobs.<job_id>.steps[*].name. However, That's just my personal opinion.

I will raise this in our team meeting this week to see if there's consensus in the team on what we should use. Stay tuned, I'll come back with an answer!

@Simran-B
Copy link
Contributor Author

Correct, there is not a single standard and both JSONPath and jq are about JSON, not YAML. On the other hand, YAML is a superset of JSON, so it's not too far off.

I like the idea of using [*]. as in yq, a true YAML tool. Since we are both not fond of .*, I'll update the PR to the currently preferred solution and am looking forward to the outcome of your team discussion,

@lucascosti
Copy link
Contributor

We discussed it internally, and we decided that the yq convention is probably the best for now. The good thing is that adding/removing special characters from the headings doesn't change the anchor URLs, so we don't have to worry about links breaking with any of these changes. 🥳

Copy link
Contributor

@lucascosti lucascosti left a comment

Choose a reason for hiding this comment

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

Thanks soooo much, @Simran-B for this marvelous work! 🙇‍♂️ This will help people understand the syntax a lot better than the misleading paths we have at the moment!

This looks great, so I'll merge it in! 🚢

@lucascosti lucascosti merged commit 25284f5 into github:main Dec 16, 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.

@Simran-B
Copy link
Contributor Author

Simran-B commented Dec 16, 2020

Thanks @lucascosti!

The good thing is that adding/removing special characters from the headings doesn't change the anchor URLs

It struck me this morning that I didn't check whether it would break existing anchor links. Great to hear that it doesn't!

Is there a policy how to handle cases where links do get broken? Should one add <a name="former-anchor-slug"></a>? I find examples of that in two files. This seems to be preferred over <a id="..."/> and <a id="..."></a>.

On another related note, is it worth to look into avoiding trailing hyphens in anchor slugs? There are a few pages with headlines like:

## GitHub Docs <!-- omit in toc -->

The space between title and comment is not stripped away by the slugger it seems, and the anchor link ends up being #github-docs-. Removing the space would make it harder to read:

## GitHub Docs<!-- omit in toc -->

So if I were to change this, I would keep the space and instead adjust the slugger behavior. To not break existing anchor links, I would make it:

<a name="github-docs-"></a>
## GitHub Docs <!-- omit in toc -->

There are only 11 instances of <!-- omit in toc -->, so not that many <a>s to add, but on the other hand, it's just 11 anchor slugs with trailing hyphens, so not sure if it's worth to look into.

jnidzwetzki pushed a commit to jnidzwetzki/docs that referenced this pull request Oct 6, 2022
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 content This issue or pull request belongs to the Docs Content 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.

4 participants