cjoudrey / graphql-schema-linter

Validate GraphQL schema definitions against a set of rules
MIT License
694 stars 62 forks source link

Is there any plan to add initialisms rule? #283

Open dengliu opened 3 years ago

dengliu commented 3 years ago

Is there any plan to add initialisms rule?

Words in names that are initialisms or acronyms (e.g. "URL" or "NATO") have a consistent case. For example, "URL" should appear as "URL" or "url" (as in "urlPony", or "URLPony"), never as "Url". As an example: ServeHTTP not ServeHttp. For identifiers with multiple initialized "words", use for example "xmlHTTPRequest" or "XMLHTTPRequest".

This rule also applies to "ID" when it is short for "identifier" (which is pretty much all cases when it's not the "id" as in "ego", "superego"), so write "appID" instead of "appId".

https://github.com/golang/go/wiki/CodeReviewComments#initialisms

cjoudrey commented 3 years ago

👋 @dengliu, I could see this being useful for rules like FieldsAreCamelCased. I'm guessing this is why you're bring this up?

I think we'd want to let people configure their initialisms and then we can use that configuration within the rule when applicable.

Given that there are multiple rules that might need this information (e.g. FieldsAreCamelCased, TypesAreCapitalized, InputObjectValuesAreCamelCased, ...), it might make sense to make this a global configuration instead of forcing people to copy/paste the same initialisms in each rule's options.

Here's an example rule that leverages the global configuration: https://github.com/cjoudrey/graphql-schema-linter/blob/c2876f3b93bd2a262c2af838f17cca5e21c23848/src/rules/fields_have_descriptions.js#L4-L12

If you'd like to help contribute this feature, I'd be happy to discuss ideas here and 👀 review your implementation.

dengliu commented 3 years ago

thanks for the quick @cjoudrey I'd love to contribute a bit but I cannot make a hard commitment for this as I am not a JS guy.

I agree to give the flexibility for users to define their own list of initialisms. However, I think as a first step, we can start with some common initialisms defined in lib/config.js , just follow the golint example below: https://github.com/golang/lint/blob/master/lint.go#L770-L800

WDYT?

cjoudrey commented 3 years ago

However, I think as a first step, we can start with some common initialisms defined in lib/config.js , just follow the golint example below: https://github.com/golang/lint/blob/master/lint.go#L770-L800

Yeah, that's definitely a second option. I'm not too sure about that approach as this might force people to change their schema and changing the casing of a type / field might be considered a breaking change for some implementations.

e.g. If we decide URL is now a initialisms by default, any schema today that uses Url would get a linter error as soon as they upgrade to the new version of graphql-schema-linter and they'd be forced to rename to URL which might require a lot of changes on their end (and possibly a breaking change to the API).

dengliu commented 3 years ago

agree. Ideally would like to introduce the initialisms config map as the example below. So if we have that, do we need t update multiple rules to honor this list (e.g. FieldsAreCamelCased, TypesAreCapitalized, InputObjectValuesAreCamelCased, ...) Or if there is a global rule that is checked before the rest of multiple rules. (Not sure how the rule checking order is implemented.)

module.exports = {
  rules: ['enum-values-sorted-alphabetically'],
  schemaPaths: ['path/to/my/schema/files/**.graphql'],
  customRulePaths: ['path/to/my/custom/rules/*.js'],
  rulesOptions: {
    'enum-values-sorted-alphabetically': { sortOrder: 'lexicographical' }
  },
  initialisms: {
    'ID':    true,
    'IP':    true,
    'JSON':  true
  }
};
cjoudrey commented 3 years ago

Oh, that's an interesting idea too. 🤔

I was just looking at the rules I mentioned above, and they seem to be pretty loose with the definition of camel case:

https://github.com/cjoudrey/graphql-schema-linter/blob/c2876f3b93bd2a262c2af838f17cca5e21c23848/test/rules/fields_are_camel_cased.js#L16-L17

So perhaps we could do this as a new rule that checks initialisms everywhere.

Nitpick about your configuration example, I think we could probably go with an array of strings, unless the boolean value has meaning.

dengliu commented 3 years ago

Oh, the reason to use map instead of list is to have the the developer to have options to know what are the common initialism available to config instead having them to spent too much time to compile this list. It's easier for them to decide wether a given initialism need to be on or off than think of a list of them.