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

feat(new-rule): ibm-operation-summary-length #663

Merged
merged 1 commit into from
May 9, 2024
Merged

Conversation

dpopp07
Copy link
Member

@dpopp07 dpopp07 commented May 8, 2024

PR summary

Adds a rule that verifies operation summaries are 80 characters or less in length, based on the summary writing guidance in the API Handbook.

A question for the PR reviewers. We already have these existing rules:

  • ibm-operation-summary (checks that a summary exists on every operation)
  • ibm-summary-sentence-style (checks that a summary does not end with a period)

Should this check be merged into one of the existing rules or does it belong as its own rule? The granularity is nice for configuration but it might be confusing to have too many rules doing such similar things. What do y'all think?

PR Checklist

General checklist

Please make sure that your PR fulfills the following requirements:

  • The commit message follows the Angular Commit Message Guidelines.
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Dependencies have been updated as needed
  • .secrets.baseline updated as needed?

Checklist for adding a new validation rule:

  • Added new validation rule definition (packages/ruleset/src/rules/*.js, index.js)
  • If necessary, added new validation rule implementation (packages/ruleset/src/functions/*.js, updated index.js)
  • Added new rule to default configuration (packages/ruleset/src/ibm-oas.js)
  • Added tests for new rule (packages/ruleset/test/*.test.js)
  • Added docs for new rule (docs/ibm-cloud-rules.md)

@dpopp07 dpopp07 requested review from pyrooka and padamstx May 8, 2024 15:05
@CLAassistant
Copy link

CLAassistant commented May 8, 2024

CLA assistant check
All committers have signed the CLA.

Adds a rule that verifies operation summaries are 80 characters
or less in length, based on the summary writing guidance in the
API Handbook.

Signed-off-by: Dustin Popp <dpopp07@gmail.com>
Copy link
Member

@pyrooka pyrooka left a comment

Choose a reason for hiding this comment

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

The change looks good altogether!
Re: the merging question. I think it's fine to leave this as a separate rule, but if we decide to merge it, I'd choose the ibm-operation-summary. I think it makes a bit more sense to have the null || len(summary) > 80 logic in the same rule. Also, the sentence style can be changed in the future, by adding new criteria, for example "start with capital letter".

Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

Looks good.
Since we already have two existing rules for summary-related stuff, my vote would be to keep this one separate as well.

@dpopp07 dpopp07 merged commit 901cc1a into main May 9, 2024
4 checks passed
@dpopp07 dpopp07 deleted the dp/summary-length branch May 9, 2024 18:41
ibm-devx-sdk pushed a commit that referenced this pull request May 9, 2024
# @ibm-cloud/openapi-ruleset [1.16.0](https://github.com/IBM/openapi-validator/compare/@ibm-cloud/openapi-ruleset@1.15.9...@ibm-cloud/openapi-ruleset@1.16.0) (2024-05-09)

### Features

* **new-rule:** ibm-operation-summary-length ([#663](#663)) ([901cc1a](901cc1a))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 1.16.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@ibm-devx-sdk
Copy link

🎉 This PR is included in version 1.17.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants