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

Better converts and preserves comment descriptions #437

Merged
merged 10 commits into from
Nov 18, 2021

Conversation

vicary
Copy link
Contributor

@vicary vicary commented Oct 9, 2021

So after some months of tests and production use, I am fairly confident that my fork is good enough for a PR.

This PR adds my appsync-schema-converter to do the followings,

  1. Converts string literal descriptions into hash-comment descriptions
  2. Converts the separator of multiple interfaces into comma
  3. Removes enum descriptions
  4. Keep input field descriptions

The current codebase in serverless-appsync-plugin removes all comments from the AppSync schema, disabling the self-documenting feat of GraphQL as a whole. One may argue a publicly accessible introspection schema is a security risk, it is really useful as an internal communication tool.

I also added prettier config in package.json which matches one of the many conflicting coding style, so there are style changes to make everything consistent. Please let me know if this is not preferred, I can revert those changes back to its original state.

Please comment.

@bboure
Copy link
Collaborator

bboure commented Oct 23, 2021

Awesome @vicary 🎉

Sorry for the late response. I somehow missed your PR.
It looks good to me and sounds like a great addition.

Regarding Prettier, I actually just added it in #438
Sorry for stepping on your toes and generating merge conflicts.

I'd be happy to accept this PR when merge conflicts are fixed.

Thanks for contributing

@vicary
Copy link
Contributor Author

vicary commented Nov 5, 2021

@bboure alright, it's kinda messy but it's done.

Conflicting lock files got re-installed lazily with no shame, expect minor upgrades but I made sure it's all greens in the test.

A lot of lint fixes snuck in the history because of the new prettier config, let's pretend it's like that from the beginning 😄

I merged my prettier section in package.json into your .prettierrc. Please forgive my OCD for sorting the keys there.

Copy link
Collaborator

@bboure bboure left a comment

Choose a reason for hiding this comment

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

Thank you @vicary and sorry for the late review.
I was wondering why change the prettier config, especially to 120? Any reason?
Most projects use the default (80) or 100.

Side note (that does not have to have any consequences on the final decision):
I personally use panes (like most of us I think) and the max chars I see in one pane on my setup is around 105. So I would have to start scrolling 🙁

.prettierrc Outdated
"tabWidth": 2,
"useTabs": false,
"endOfLine": "auto",
"printWidth": 120,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, why 120?

@vicary
Copy link
Contributor Author

vicary commented Nov 14, 2021

@bboure Sorry for not aware of the common widths of other IDEs, I have been using Sublime Text and VS Code and have been using [80, 120] for my whole life.

120 is picked purely because it makes less of a change compared to the 80 one.

If you like 80 more I don't mind re-formatting it one more time, it's your call.

@bboure
Copy link
Collaborator

bboure commented Nov 14, 2021

If I'm not mistaken, 80 was the default after I merged #438 (prior to that, it was a bit of a mess, indeed 😄 )

Try changing it back to 80 and you should see less changes in the PR. (It would also make it easier to understand what changed)

@vicary
Copy link
Contributor Author

vicary commented Nov 14, 2021

I must have overlooked, let me change that back in the morning.

@vicary
Copy link
Contributor Author

vicary commented Nov 15, 2021

@bboure It's done, printWidth is now 80.

@bboure bboure merged commit bc93c3c into sid88in:master Nov 18, 2021
@bboure
Copy link
Collaborator

bboure commented Nov 18, 2021

LGTM! 🎉

Thanks again!

bboure added a commit that referenced this pull request Feb 12, 2022
vicary added a commit to vicary/serverless-appsync-plugin that referenced this pull request Mar 22, 2023
vicary pushed a commit to vicary/serverless-appsync-plugin that referenced this pull request Mar 22, 2023
vicary added a commit to vicary/serverless-appsync-plugin that referenced this pull request Jun 10, 2023
vicary pushed a commit to vicary/serverless-appsync-plugin that referenced this pull request Jun 10, 2023
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.

2 participants