-
Notifications
You must be signed in to change notification settings - Fork 189
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
Conversation
Awesome @vicary 🎉 Sorry for the late response. I somehow missed your PR. Regarding Prettier, I actually just added it in #438 I'd be happy to accept this PR when merge conflicts are fixed. Thanks for contributing |
@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 |
There was a problem hiding this 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, why 120?
@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 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. |
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) |
I must have overlooked, let me change that back in the morning. |
@bboure It's done, |
LGTM! 🎉 Thanks again! |
…)" This reverts commit bc93c3c.
…)" This reverts commit bc93c3c.
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,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.