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

style(specs): add out-of-line-one-of rule (and allOf and anyOf) APIC-418 #512

Merged
merged 3 commits into from
May 19, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented May 19, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-418

Reuse the logic of the out-of-line-enum rule to create rules for oneOf, allOf, anyOf.
The specs already complied with this rule.

🧪 Test

CI

@millotp millotp self-assigned this May 19, 2022
@netlify
Copy link

netlify bot commented May 19, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 1a67fe6
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/62862139488e640008866682

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 19, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

@millotp millotp requested review from a team, eunjae-lee and damcou and removed request for a team May 19, 2022 08:54
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Noice

Comment on lines 10 to 13
'out-of-line-enum': outOfLineEnum,
'out-of-line-one-of': outOfLineOneOf,
'out-of-line-all-of': outOfLineAllOf,
'out-of-line-any-of': outOfLineAnyOf,
Copy link
Member

Choose a reason for hiding this comment

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

I believe it works but maybe we can remove the extra outOfLineXXX files to call the createOutOfLineRule here, I feel like it would be easier to understand

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

create one rule that handle all of them ? It's easier to do it that way, and very flexible, maybe we want do disable it for anyOf at some point

Copy link
Member

Choose a reason for hiding this comment

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

No sorry I meant that since those function just make a call with a different parameter, maybe we could have

Suggested change
'out-of-line-enum': outOfLineEnum,
'out-of-line-one-of': outOfLineOneOf,
'out-of-line-all-of': outOfLineAllOf,
'out-of-line-any-of': outOfLineAnyOf,
'out-of-line-enum': createOutOfLineRule({ property: 'enum' }),
'out-of-line-one-of': createOutOfLineRule({ property: 'oneOf' }),
'out-of-line-all-of': createOutOfLineRule({ property: 'allOf' }),
'out-of-line-any-of': createOutOfLineRule({ property: 'anyOf' }),

So we don't have to go to an empty file :D

Copy link
Collaborator Author

@millotp millotp May 19, 2022

Choose a reason for hiding this comment

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

are you sure this is possible ?
those functions are ts, here it's js

Copy link
Member

Choose a reason for hiding this comment

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

are you sure this is possible ?

We return a const so I'd assume it does yes

those functions are ts, here it's js

The index is a ts file too

Copy link
Member

Choose a reason for hiding this comment

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

(I did not tried locally I just guessed D: )

Copy link
Collaborator Author

@millotp millotp May 19, 2022

Choose a reason for hiding this comment

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

the index.ts file is transpiled in js and used as a package, not imported directly

Copy link
Member

Choose a reason for hiding this comment

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

Yep but in the end this object will hold a rule value, I mean I don't see why it wouldn't work the same way

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I though you were talking about the .eslintrc.js file sorry all my comments makes no sense

Copy link
Member

Choose a reason for hiding this comment

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

no prob!!

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

You could rename the test file eslint/tests/outOfLineEnum.test.ts to eslint/tests/createOutOfLineRule.test.ts

and test each case we support in it

export function createOutOfLineRule({
property,
description = `${property} must be out of line, not nested inside properties`,
messageId = `${property}OutOfLine`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
messageId = `${property}OutOfLine`,
messageId = `${property}NotOutOfLine`,

Shouldn't it be that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know, should the message tell you what's expected or what happened ?

Copy link
Member

@shortcuts shortcuts May 19, 2022

Choose a reason for hiding this comment

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

I just based my comment on what's existing and tested

I believe since the message is must be out of line, the error thrown should be that it's notOutOfLine but up to you!

@shortcuts
Copy link
Member

shortcuts commented May 19, 2022

(Spammer)

I realized the tests are not ran in this PR, we should update the run condition of scripts

Copy link
Member

@shortcuts shortcuts 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!!

@millotp millotp merged commit 6361b60 into main May 19, 2022
@millotp millotp deleted the style/outoflineoneof branch May 19, 2022 11:36
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.

3 participants