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: introduce Rule #1112

Merged
merged 1 commit into from
May 3, 2020
Merged

refactor: introduce Rule #1112

merged 1 commit into from
May 3, 2020

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Apr 20, 2020

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

There are not so amny changes taking aplce here.
The only actual notable changes are:

  • linter and runners have been moved and their implementations have been slightly modified, but semantically they are almost equal
  • introduced Rule that is a more unified IRunRule that is more convienent to use during the linting phase, i.e. exposes enabled, matchesFormat, etc. Plus, the data types are more strict, so less duck-typing or other checks are required

This one of the chunks of "json-path on rails" (#437) chore.
I'll split it into smaller pieces, as certain changes are not directly related.

@P0lip P0lip self-assigned this Apr 20, 2020
@@ -39,7 +39,7 @@ export const buildTestSpectralWithAsyncApiRule = async (ruleName: string): Promi
expect(Object.keys(s.rules)).toEqual([ruleName]);

const rule = s.rules[ruleName];
expect(rule.recommended).not.toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Bringing back memories! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it's going backwards. I was pointing this out as a mistake, sorry for not being clear, We removed enabled so why does this test talk about enabled instead of recommended?

Copy link
Contributor Author

@P0lip P0lip May 4, 2020

Choose a reason for hiding this comment

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

enabled is internal, nothing exposed to end user.
Basically, it's a combination of recommended + severity, but it's something end user will be unaware of.
This is a check we had to do in the lining process to verify whether a given rule should be executed or not, so I simply extracted it to a single spot for our own convenience.

tl;dr I didn't re-add the enabled on ruleset rule, it's an internal rule instance (former IRunRule). User has no access to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@philsturgeon I think @P0lip's change is sound. recommended is a rule property in the context of the ruleset. Once loaded, it's of little use. However, enabled is needed from the rule engine standpoint.

And that's one of thing I do like with this PR. It disambiguates a RulesetRule from a loaded one.

FWIW, I'd have taken this one step further and kept the initial severity level from the ruleset (without turning it off when not recommended), which would help on the test standpoint and avoid code like

...((rule as IRule).recommended === false && { severity: -1 }),

@P0lip P0lip force-pushed the refactor/introduce-rule branch from 6bbe0f4 to 71d9925 Compare May 1, 2020 21:43
@P0lip P0lip force-pushed the refactor/introduce-rule branch from 71d9925 to c40094b Compare May 1, 2020 22:02
@P0lip P0lip added the chore label May 1, 2020
@P0lip P0lip marked this pull request as ready for review May 1, 2020 22:02
@P0lip P0lip requested a review from nulltoken May 1, 2020 22:02
Copy link
Contributor

@nulltoken nulltoken left a comment

Choose a reason for hiding this comment

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

Very nice!

@nulltoken nulltoken merged commit 13999e0 into develop May 3, 2020
@nulltoken nulltoken deleted the refactor/introduce-rule branch May 3, 2020 07:43
@P0lip P0lip mentioned this pull request May 3, 2020
4 tasks
@@ -20,17 +19,17 @@ const removeAllRulesBut = (spectral: Spectral, ruleName: string) => {

const rawRule = asyncApiRules[ruleName];

const patchedRule: IRule = Object.assign(rule1, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we wanted to try and avoid casting, but this removes a type declaration and introduces a cast.

@@ -735,7 +735,7 @@ console.log(this.cache.get('test') || this.cache.set('test', []).get('test'));

expect(Object.keys(spectral.rules)).toHaveLength(3);

expect(Object.entries(spectral.rules).map(([name, rule]) => [name, rule.recommended])).toEqual([
Copy link
Contributor

Choose a reason for hiding this comment

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

AGH

@philsturgeon
Copy link
Contributor

philsturgeon commented May 4, 2020 via email

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.

3 participants