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

refactor: replace info object check with a custom Spectral rule #255

Closed
wants to merge 1 commit into from

Conversation

barrett-schonefeld
Copy link
Contributor

Outstanding problems to be resolved:

  • When a user provides a Spectral configuration file, our custom rules will not be invoked. Considerations
    • Need to ensure our custom rules are invoked when the user provides a Spectral configuration file.
    • Need to ensure the custom rules have access to their corresponding custom functions. Need to consider that user's Spectral configuration file may be in different directories and adjust the path to the custom functions directory.
    • Need to ensure users are able to adjust the severity of custom rules. Users will be able to configure previously "builtin" rules.
    • May want to adjust the way we process user's Spectral config file to, first, process our default Spectral configuration file then load the user's Spectral configuration file and overwrite the default values. Simply put, I think it'd be better to merge the user's Spectral configuration file into the default Spectral configuration file.
    • Not my idea but a good idea: Have a Spectral config file updater to give users a command to add new rules to their Spectral configuration file.

Purpose:

  • Simplify the validator ruleset by replacing good candidates with an equivalent rule
  • Establish a pattern in the validator for adding custom spectral rules and custom spectral functions

Changes:

  • Add custom spectral rule to ensure that an info object is provided in the spec
  • Add custom spectral function
  • Remove in-code validation of info object

Tests:

  • Move existing tests to ensure the equivalent spectral rule produces the same behavior

@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@0b2032d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #255   +/-   ##
=======================================
  Coverage        ?   92.25%           
=======================================
  Files           ?       70           
  Lines           ?     2389           
  Branches        ?      618           
=======================================
  Hits            ?     2204           
  Misses          ?      153           
  Partials        ?       32           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b2032d...7779d7b. Read the comment docs.

@mkistler
Copy link
Contributor

mkistler commented Mar 3, 2021

I think the general approach to adding custom rules and custom functions in this PR looks sound, but I don't think the rule to check for an info object is the right application. The info object is required by the OpenAPI 3.0 specification, so the Spectral oas3-schema rule should issue an appropriate error if it is missing or is not an object. So the rule defined here would be redundant with that Spectral rule -- something we should avoid.

Let's identify another rule to implement that won't duplicate a Spectral rule to prove out this approach.

@barrett-schonefeld barrett-schonefeld marked this pull request as draft March 3, 2021 15:05
Purpose:
- Simplify the validator ruleset by replacing good candidates with an equivalent rule
- Establish a pattern in the validator for adding custom spectral rules and custom spectral functions

Changes:
- Add custom spectral rule to ensure that an info object is provided in the spec
- Add custom spectral function
- Remove in-code validation of info object

Tests:
- Move existing tests to ensure the equivalent spectral rule produces the same behavior
@barrett-schonefeld
Copy link
Contributor Author

Closing this PR in favor of #258 because #258 both establishes a pattern for custom Spectral rules and implements a custom Spectral rule for which there is no existing, equivalent Spectral rule.

@barrett-schonefeld barrett-schonefeld deleted the convert-info-check-to-spectral branch March 5, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants