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

Create new lone-executable-definition rule #1316

Merged
merged 7 commits into from
Dec 20, 2022
Merged

Create new lone-executable-definition rule #1316

merged 7 commits into from
Dec 20, 2022

Conversation

FloEdelmann
Copy link
Contributor

Description

Adds the suggested rule from #1311 to require one file per query/fragment/mutation/subscription. Fixes #1311.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I've created new unit tests for the rule, and also ran npm test to be sure that I did not break anything.

Test Environment:

  • OS: Ubuntu 22.04
  • NodeJS: 16.18.0

Checklist:

  • I have followed the
    CONTRIBUTING doc and the
    style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests and linter rules pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

The latest commit (b0b0cc8) were automatically generated with npm run prettier. I'm happy to revert it if required.

@changeset-bot
Copy link

changeset-bot bot commented Dec 20, 2022

🦋 Changeset detected

Latest commit: 866828c

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

This PR includes changesets to release 1 package
Name Type
@graphql-eslint/eslint-plugin Minor

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

@dimaMachina
Copy link
Owner

Amazing! Hope it was fun! Will be part of next release 🎉

@FloEdelmann
Copy link
Contributor Author

It was! And actually relatively easy to implement.

Copy link
Owner

@dimaMachina dimaMachina left a comment

Choose a reason for hiding this comment

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

I will fix conflicts

@@ -24,7 +24,7 @@ const uppercaseFirst = (string: string) => string.slice(0, 1).toUpperCase() + st
const isDefinitionNode = (node: { kind: string }): node is DefinitionESTreeNode =>
node.kind === Kind.OPERATION_DEFINITION || node.kind === Kind.FRAGMENT_DEFINITION;

const rule: GraphQLESLintRule<[LoneExecutableDefinitionConfig]> = {
export const rule: GraphQLESLintRule<[LoneExecutableDefinitionConfig]> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that this change (named instead of default export) is breaking the tests.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I already fixed it, made some refactoring to use ESLint selectors, you should like it

items: {
type: 'object',
minProperties: 1,
additionalProperties: false,
Copy link
Owner

Choose a reason for hiding this comment

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

this prop was missing to catch JSON schema mistake

minProperties: 1,
additionalProperties: false,
properties: {
ignore: {
Copy link
Owner

Choose a reason for hiding this comment

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

previously it was ignores, but you passed ignore to ESLint

Comment on lines +81 to +100
':matches(OperationDefinition, FragmentDefinition)'(node: DefinitionESTreeNode) {
const type = 'operation' in node ? node.operation : 'fragment';
if (!ignore.has(type)) {
definitions.push({ type, node });
}
},
'Program:exit'() {
for (const { node, type } of definitions.slice(1) /* ignore first definition */) {
let name = pascalCase(type);
const definitionName = node.name?.value;
if (definitionName) {
name += ` "${definitionName}"`;
}
context.report({
loc: node.name?.loc || getLocation(node.loc.start, type),
messageId: RULE_ID,
data: { name },
});
}
},
Copy link
Owner

Choose a reason for hiding this comment

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

here I made some refactoring, looks cleanly now

const pascalCase = (str: string): string =>
export const pascalCase = (str: string): string =>
Copy link
Owner

Choose a reason for hiding this comment

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

we can use this function instead manual upperCase

@dimaMachina dimaMachina merged commit 1f21fc8 into dimaMachina:master Dec 20, 2022
@FloEdelmann FloEdelmann deleted the lone-executable-definition branch December 20, 2022 14:15
dimaMachina added a commit that referenced this pull request Dec 20, 2022
* Create new `lone-executable-definition` rule

* Generate docs and configs

* Other prettier changes

* Allow ignoring first definition

* refactor

* fix typecheck on GraphQL 15

* add changeset

Co-authored-by: Dimitri POSTOLOV <dmytropostolov@gmail.com>
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.

New rule lone-executable-definition
2 participants