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

Improve naming of per-format rules sections #1723

Open
lornajane opened this issue Sep 10, 2024 · 10 comments
Open

Improve naming of per-format rules sections #1723

lornajane opened this issue Sep 10, 2024 · 10 comments

Comments

@lornajane
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Most users of Redocly CLI list rules under rules but as we see more customers using multiple API formats in their projects, we need to document the per-format rules config entries so that different formats can be linted using different rulesets. The per-format rules config entries currently are:

  • oas2Rules
  • oas3_0Rules
  • oas3_1Rules
  • async2Rules
  • async3Rules
  • arazzoRules

(equivalents exist for *Preprocessors and *Decorators)

Proposal: rename these config settings to something that fits our config naming scheme

In an ideal world, we could offer something like this:

rules:
  openapi-30:
    security-defined: warn
  arazzo-10:
    parameters-unique: error

Describe alternatives you've considered

Just make the config entry names more sane and leave them at the top level:

  • oas2Rules -> rules-openapi-20
  • oas3_0Rules -> rules-openapi-30
  • oas3_1Rules -> rules-openapi-31
  • async2Rules -> rules-asyncapi-26
  • async3Rules -> rules-asyncapi-30
  • arazzoRules -> rules-arazzo-10

Additional context

From a discussion with @DmitryAnansky and @tatomyr about how to thoroughly support and document our multi-format linting features.

@tatomyr
Copy link
Contributor

tatomyr commented Sep 11, 2024

I vote for the top-level, but I don't like naming like openapi-30... Maybe openapi-3-0 will work better?

@lornajane
Copy link
Contributor Author

@adamaltman or @RomanHotsiy could you comment on the requested renaming?

@RomanHotsiy
Copy link
Member

It sounds good. I agree with @tatomyr though on the naming, I prefer openapi-3-0 or we can even do openapi-3.0. Both yaml and json support keys with dots.

@tatomyr
Copy link
Contributor

tatomyr commented Sep 16, 2024

@RomanHotsiy It looks much nicer with dots. The only issue is that I vaguely remember we had some problems with parsing features.openapi. Or am I wrong?

@lornajane
Copy link
Contributor Author

I think it would probably work, but it's confusing to use dots inside a yaml key IMO, just because there's a convention of using it as a separator. For example info.description , we use this pattern ourselves in Redoc rendering too. So I discarded the dot and thought that the extra dash wasn't needed with every version being one major digit and one minor one. Happy to compromise on the two-dash format though!

Will we refactor this throughout the code, or as an alias for parsing the config in? We use this type of pattern in plugins too (as well as internally)

@tatomyr
Copy link
Contributor

tatomyr commented Sep 17, 2024

Will we refactor this throughout the code, or as an alias for parsing the config in?

For me, it doesn't make much sense to refactor it deeply, especially taking into account that we will have to use some artificial names for the variables anyway.

@adamaltman
Copy link
Member

adamaltman commented Oct 7, 2024

I classify this issue differently. I propose a different format which would rely on the verbose syntax of each rule:

rules:
  info-description:
    severity: error
    specs:
       - openapi3.0
       - asyncapi3.0
       - arazzo1.0

This technique could be used for configurable rules too:

rules:
  rule/limit-is-integer:
    subject:
      type: Schema
      property: type
    assertions:
      const: integer
    where:
      - subject:
          type: Parameter
          property: name
        assertions:
          const: limit
    specs:
       - openapi3.0

@lornajane
Copy link
Contributor Author

I think the current approach of listing which rules should apply to which format/family of APIs makes good sense, since different teams might manage the event-driven, legacy, or Arazzo types. We also design, maintain and document the rules on a per-specification/format basis, so configuring things along the same lines seems sensible.

The other major advantage of the existing approach is that it already works this way (when there are two ways to do things, the one that exists and is already tested and working does have some advantage points) and I think it works adequately. The only change proposed in this issue is to rename the existing top-level keys to something a bit more correct, friendly and future-proof.

@adamaltman
Copy link
Member

This is not quite accurate:

We also design, maintain and document the rules on a per-specification/format basis, so configuring things along the same lines seems sensible.

We have single rules that span specifications. Many rules span OpenAPI 3.1 and 3.0. Quite a few also include 2.0. Some span OpenAPI and AsyncAPI.

@tatomyr
Copy link
Contributor

tatomyr commented Oct 22, 2024

I propose a different format which would rely on the verbose syntax of each rule:

Then there will be no way to configure different severity levels for different specifications.
Say, a user might want info-license-strict to be an error for OAS3/3.1 but a warning (or turned off) for AsyncAPI.
Not sure how useful that might be, but still.

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

No branches or pull requests

4 participants