Draft: Reject unknown shortcode named parameters
Why is this change being made?
When using a shortcode such as
{{< team-by-manager-slug manager="cliu" role="Global Search|Core Platform" >}}
The role
named parameter is not doing anything because layouts/shortcodes/team-by-manager-slug.html
doesn't use it.
This MR updates the shortcode template to disallow unknown named parameters. If one is provided, an error will be generated.
We can see that this is a common mistake by looking at the error logs:
ERROR Invalid named parameter: role. The team-by-manager-slug shortcode recognizes: [manager department team]. Source: handbook/engineering/development/sec/govern/_index.md.
ERROR Invalid named parameter: role. The team-by-manager-slug shortcode recognizes: [manager department team]. Source: handbook/engineering/development/sec/secure/_index.md.
ERROR Invalid named parameter: role. The team-by-manager-slug shortcode recognizes: [manager department team]. Source: handbook/engineering/infrastructure/core-platform/data_stores/search/_index.md.
ERROR Invalid named parameter: role. The team-by-manager-slug shortcode recognizes: [manager department team]. Source: handbook/engineering/infrastructure/core-platform/systems/cloud-connector/_index.md.
In the usage above, the role
named parameter is currently ignored.
If this pattern is desirable, these are some of the tasks that we might want to complete before merging:
- Fix the remaining errors so the build passes.
- Perhaps eliminate the use of
range
by usingcollections.Complement
instead. -
If possible, eliminate the use of variables in(done)errorf
. - If possible, use a partial for the
.Params
verification.
Other improvements for follow-ups:
- Eliminate the use of positional parameters on all shortcodes.
- Show line number on the source.
Author and Reviewer Checklist
Please verify the check list and ensure to tick them off before the MR is merged.
-
Provided a concise title for this Merge Request (MR) -
Added a description to this MR explaining the reasons for the proposed change, per say why, not just what - Copy/paste the Slack conversation to document it for later, or upload screenshots. Verify that no confidential data is added, and the content is SAFE
-
Assign reviewers for this MR to the correct Directly Responsible Individual/s (DRI) - If the DRI for the page/s being updated isn’t immediately clear, then assign it to one of the people listed in the
Maintained by
section on the page being edited - If your manager does not have merge rights, please ask someone to merge it AFTER it has been approved by your manager in #mr-buddies
- The when to get approval handbook section explains the workflow in more detail
- If the DRI for the page/s being updated isn’t immediately clear, then assign it to one of the people listed in the
-
For transparency, share this MR with the audience that will be impacted. -
Team: For changes that affect your direct team, share in your group Slack channel -
Department: If the update affects your department, share the MR in your department Slack channel -
Company: If the update affects all (or the majority of) GitLab team members, post an update in #whats-happening-at-gitlab linking to this MR - For high-priority company-wide announcements work with the internal communications team to post the update in #company-fyi and align on a plan to circulate in additional channels like the "While You Were Iterating" Newsletter
-
Edited by Thiago Figueiró