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.

Sign up for GitHub

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

Jump to bottom

feat: rules refactoring / add separate arazzo rule-set #1800

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: rules refactoring / add separate arazzo rule-set #1800

wants to merge 2 commits into from

Conversation

DmitryAnansky
Copy link
Contributor

@DmitryAnansky DmitryAnansky commented Nov 13, 2024 •
edited
Loading

What/Why/How?

  • Refactoring rules structure.
  • Add separate Arazzo ruleset.
  • Update docs structure for rules.
  • Removed the type:none sourceDescriptions Arazzo extension.
  • Add additional lint rule for sourceDescriptions:
sourceDescriptions [Source Description Object] REQUIRED. A list of source descriptions (such as an OpenAPI description) this Arazzo Description SHALL apply to. The list MUST have at least one entry.

TODO:

  • - add docs redirects #1514

Reference

https://github.com/Redocly/redocly/issues/11343

Testing

chore(all): dummy pr to test new cli and core #12281

Screenshots (optional)

Check yourself

  • Code changed? - Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests
  • New package installed? - Tested in different environments (browser/node)

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 13, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: d8a4000

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@redocly/openapi-core Patch
@redocly/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 13, 2024 •
edited
Loading

Command Mean [ms] Min [ms] Max [ms] Relative
redocly lint packages/core/src/benchmark/benches/rebilly.yaml 981.9 ± 45.4 944.6 1101.4 1.01 ± 0.05
redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml 967.8 ± 14.0 940.3 988.4 1.00

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 13, 2024 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements 78.86% 5043/6395
🟡 Branches 67.21% 2052/3053
🟡 Functions 73.42% 834/1136
🟡 Lines 79.16% 4757/6009
Show new covered files 🐣
St.❔
 File Statements Branches Functions Lines
🟢
... / spec-arazzo.ts
 100% 100% 100% 100%
🟢
... / struct.ts
 92.41% 81.82% 100% 91.78%
🟢
... / sourceDescription-type.ts
 87.5% 50% 100% 100%
🟢
... / sourceDescriptions-not-empty.ts
 100% 100% 100% 100%
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢
... / redocly-yaml.ts
 83.33% 50% 71.43% 82.93%

Test suite run success

816 tests passing in 122 suites.

Report generated by 🧪jest coverage report action from d8a4000

@JLekawa JLekawa mentioned this pull request Nov 14, 2024
Open
@DmitryAnansky DmitryAnansky changed the title feat: add separate arazzo rule-set feat: rules refactoring / add separate arazzo rule-set Nov 15, 2024
@DmitryAnansky DmitryAnansky mentioned this pull request Nov 19, 2024
Closed
5 tasks
@DmitryAnansky DmitryAnansky force-pushed the feat/rules-refactoring branch 2 times, most recently from a281061 to c118997 Compare November 21, 2024 16:27
@DmitryAnansky DmitryAnansky marked this pull request as ready for review November 22, 2024 15:33
@DmitryAnansky DmitryAnansky requested review from a team as code owners November 22, 2024 15:33
@DmitryAnansky DmitryAnansky reopened this Nov 22, 2024
RomanHotsiy
RomanHotsiy reviewed Nov 25, 2024
View reviewed changes
Copy link
Member

@RomanHotsiy RomanHotsiy left a comment

Choose a reason for hiding this comment

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

make sure lintignore works with fallback from spec to struct

@DmitryAnansky
Copy link
Contributor Author

DmitryAnansky commented Nov 25, 2024 •
edited
Loading

lintignore

@RomanHotsiy , are you talking about --generate-ignore-file option?
Tested with spec:

Screenshot 2024-11-25 at 11 24 06

Screenshot 2024-11-25 at 11 24 12

Tested with struct:

Screenshot 2024-11-25 at 11 24 38

Screenshot 2024-11-25 at 11 24 52

Screenshot 2024-11-25 at 11 24 58

Please let me know if you have other test cases in mind.

tatomyr
tatomyr reviewed Nov 26, 2024
View reviewed changes
Copy link
Contributor

@tatomyr tatomyr left a comment

Choose a reason for hiding this comment

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

Left a few comments.

packages/core/src/config/__tests__/__snapshots__/config-resolvers.test.ts.snap Outdated Show resolved Hide resolved
packages/core/src/config/builtIn.ts
@@ -2,11 +2,12 @@ import recommended from './recommended';
import recommendedStrict from './recommended-strict';
import all from './all';
import minimal from './minimal';
import specArazzo from './spec-arazzo';
Copy link
Contributor

Choose a reason for hiding this comment

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

I was under impression that we are not going to introduce a new ruleset. What is the case for a separate one for Arazzo? As far as I understand, other rulesets already contain that rules.
cc @RomanHotsiy @adamaltman

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarified -- this is for having a ruleset that covers only what is in the Arazzo specification.

I have another idea. What if we introduce a general spec ruleset that follows all supported specifications, not only Arazzo? The tool is smart enough to apply only the related rules depending on the flavor.
@RomanHotsiy, @adamaltman what do you think?

packages/core/src/config/config.ts Outdated Show resolved Hide resolved
packages/core/src/config/config.ts Outdated Show resolved Hide resolved
packages/core/src/rules/arazzo/sourceDescription-type.ts Show resolved Hide resolved
packages/core/src/types/redocly-yaml.ts Outdated Show resolved Hide resolved
packages/core/src/rules/arazzo/index.ts Outdated Show resolved Hide resolved
packages/core/src/types/redocly-yaml.ts Show resolved Hide resolved
packages/core/src/utils.ts Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@DmitryAnansky DmitryAnansky force-pushed the feat/rules-refactoring branch 2 times, most recently from 348e049 to 1b583f0 Compare November 28, 2024 13:14
@tatomyr tatomyr mentioned this pull request Dec 2, 2024
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JLekawa JLekawa JLekawa approved these changes

@adamaltman adamaltman adamaltman approved these changes

@RomanHotsiy RomanHotsiy Awaiting requested review from RomanHotsiy

@tatomyr tatomyr Awaiting requested review from tatomyr

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
deploy_s3_sandbox
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DmitryAnansky @adamaltman @RomanHotsiy @tatomyr @JLekawa