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

adding feature to generate categories and disable the generation of the schema category and homepage #358

Merged
merged 53 commits into from
Nov 10, 2021

Conversation

coder2034
Copy link
Contributor

Motivation:
This PR contains two changes. One is to add categorization to provide for a better developer experience. For a large graphql schema, the list for graphql types can become overwhelming and having categories will make it easier for developers to find the information they want. Currently, graphql-markdown breaks documentation into mutations, interfaces, enums, etc. This proposed change will allow us to separate those types into specified categories. For example, if we were running an education management company, we could have categories like Grades, Courses, and Registration Information. Within those categories, we would have mutations, interfaces, enums, etc instead of all the types being in one list.

Change 1:
To accomplish the categorization, we would add a directive to a certain type and specify the category. For example, if I want a query to be placed under the Courses category, we can add @doc(category: ‘Courses’). We would need to define the doc directive which I've outlined below. The directive name could be anything the developer wants.

directive @doc(category: String!) on ENUM | INPUT_OBJECT | INTERFACE | OBJECT | SCALAR | UNION | FIELD_DEFINITION

type Query @doc(category: “Courses”) {
 	classes: [String!]
}

To enable this feature, the developer just has to specify the values for groupByDirective and fieldInDirective in docusaurus.config.js or as cli options. For the above example, groupByDirective would be doc and fieldInDirective would be category.

Change 2:
It would be great if there was a way to disable the automatic generating of the Schema category and the default landing page: generated.md. I added an option for developers to specify a value of false for baseURL and homepagelocationLocation which will not generate the Schema category and the homepage.

@coder2034 coder2034 requested a review from edno as a code owner October 21, 2021 05:14
@edno
Copy link
Member

edno commented Oct 21, 2021

Thank you, @coder2034, for submitting this new feature.

I will do a deep review later in the week. But, I have a first quick feedback:

  • disable the automatic generating of the Schema category default landing page
    Do you mean hiding "Schema documentation" from the sidebar?

  • What is the rationale for making those new options available through command line, ie not only in the plugin configuration?

  • I fixed the Github Actions that were not triggered for external PRs, and pushed this change into your branch (commit f615ddc).

  • Please add tests to your PR. Note that your current PR failed Test Code Base because it decreases the code coverage.

  • Could you add an example of a GraphQL schema matching Change 1 into the folder tests/__data__.

@edno edno added the enhancement New feature or request label Oct 21, 2021
@coder2034
Copy link
Contributor Author

Hey Edno!

  1. Yes, I mean not generating "Schema Documentation" in the sidebar. This would be the choice of the developer.

  2. Having those options available through the command line will make it easier to generate documentation for multiple schemas by allowing developers to set up a script where they can just specify all the commands to generate each documentation set instead of updating docusaurus.config.js for each schema. For example, let's say we want to generate documentation for two separate schemas Author.graphql and Book.graphql.
    We can put the commands below in a script and run them:
    npx docusaurus graphql-to-doc --root ./docs/GraphQL/Book/ --link /docs/next/GraphQL/Book/ --schema schema/Book.graphql -gbd doc -fg category
    npx docusaurus graphql-to-doc --root ./docs/GraphQL/Author/ --link /docs/next/GraphQL/Author/ --schema schema/Author.graphql -gbd doc -fg category

  3. I've added tests!

Please let me know of any improvements I can make! Thank you!

src/lib/printer.js Outdated Show resolved Hide resolved
src/lib/utils.js Outdated Show resolved Hide resolved
src/lib/utils.js Outdated Show resolved Hide resolved
src/lib/utils.js Outdated Show resolved Hide resolved
src/lib/utils.js Outdated Show resolved Hide resolved
src/lib/utils.js Outdated Show resolved Hide resolved
src/lib/generator.js Outdated Show resolved Hide resolved
src/lib/generator.js Outdated Show resolved Hide resolved
src/lib/printer.js Outdated Show resolved Hide resolved
@edno
Copy link
Member

edno commented Oct 23, 2021

@coder2034 - I like the overall feature, and the way you choose to leave the directive open. It will definitively be part of the package.

I did a first code review, with some recommendations.

@coder2034
Copy link
Contributor Author

Thank you for the feedback! I've made some updates and added some comments. Please let me know what you think and if there is anything that I can make better.

src/lib/categoryInfo.js Outdated Show resolved Hide resolved
src/lib/categoryInfo.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/lib/categoryInfo.js Outdated Show resolved Hide resolved
src/lib/categoryInfo.js Outdated Show resolved Hide resolved
src/lib/printer.js Outdated Show resolved Hide resolved
tests/integration/lib/generator.spec.js Outdated Show resolved Hide resolved
tests/integration/lib/generator.spec.js Outdated Show resolved Hide resolved
tests/integration/lib/generator.spec.js Outdated Show resolved Hide resolved
@edno
Copy link
Member

edno commented Oct 25, 2021

Thank you for your patience. The codebase of the v1 is not always easy to understand. And, so far, you did a good job 💪

The comments are more structural this time, but we are getting closer to get this feature merged.

@edno
Copy link
Member

edno commented Oct 25, 2021

Let me know if you face issues with resolving conflicts while merging main into this PR. I can look at those later this week.

@edno edno added this to the 1.6.0 milestone Nov 3, 2021
@coder2034
Copy link
Contributor Author

coder2034 commented Nov 5, 2021

Hey Edno! I've made the following changes:

  1. Replaced Exception with Error in src/index.js on line 74.
  2. Move parseOptionGroupByDirective to grouping-info.js
  3. Fixed allDirective typo in src/lib/grouping-info.js on line 14.
  4. Updated constructor in grouping-info.js to have two arguments: rootType and groupByDirective
  5. Refactored function that finds group in directive by replacing filter with find
  6. Changed name of CategoryInfo to GroupInfo
  7. Refactor link generation in printer.js by using hasProperty function
  8. Refactored renderer.js with the proposed changes so no empty folders will be generated
  9. Add expect.assertions statement in generator.spec.js
  10. Refactor lines 38 to 41 of renderer.js to use convertArrayToObject function
  11. Replaced Miscellaneous with misc as fallback
  12. Updated readme with documentation

I also updated snapshots for the integration tests in generator.spec.js. After adding the code which prevents empty folders from being written, the test failed so I updated it to reflect the current codebase. The folder representing Unions was not being generated anymore because it was not specified in tweet.graphql but it was still being referenced in the old snapshot. From my understanding, this is the new intended behavior.

edno
edno previously approved these changes Nov 8, 2021
src/index.js Outdated Show resolved Hide resolved
src/lib/grouping-info.js Outdated Show resolved Hide resolved
@edno
Copy link
Member

edno commented Nov 8, 2021

2 comments but overall the PR is ready.

Regarding the union in the tweet schema, I will review this after this merge in a separate PR.

Note that the failing smoke test is due to the fact that the readme file is used for rendering the pages. For this PR, I will ignore this failure, and I will fix it in a separate PR.

Great work on this feature, this is the biggest contribution on this project so far 👏

@ghost
Copy link

ghost commented Nov 9, 2021

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@coder2034
Copy link
Contributor Author

Thank you for all your feedback and help throughout this process! I appreciate it!
I've pushed up the changes you've suggested regarding the static method parseOption and also renaming the grouping-info.js file on branch https://github.com/coder2034/graphql-markdown/tree/chore/refactor_categorization_feature. Should I make a PR for that?

@edno
Copy link
Member

edno commented Nov 9, 2021

@coder2034 - You should be able to push those changes in the current branch (this PR), then I will trigger the merge.

@sonarcloud
Copy link

sonarcloud bot commented Nov 9, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@coder2034
Copy link
Contributor Author

I've made the changes and made the method to parse the category information into a static method and also renamed grouping-info.js to group-info.js.

Copy link
Member

@edno edno left a comment

Choose a reason for hiding this comment

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

Merging this PR.

The feature will be released in 1.6.0 early next week.

Thank you @coder2034 for this contribution.

@edno edno merged commit bc3b0a5 into graphql-markdown:main Nov 10, 2021
@coder2034
Copy link
Contributor Author

Thanks Edno!

@edno
Copy link
Member

edno commented Nov 16, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants