Put comments behind an option in formatter #271
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In vektah#263, we introduced parsing of comments, as well as support for them in the formatter. In some cases this is surely useful, but in others it's just bloat. (And as I describe in Khan/genqlient#282, it may even be a problem in some cases which depended on the fact that formatting the query didn't include comments.) In this commit I introduce an option to control whether comments are formatted. I set the default to false (i.e. restoring the previous behavior if no options are set), because adding this felt to me like a breaking change, and because it seems to me like the more common usage. `WithComments()` restores the behavior added in vektah#263. If others disagree I'm happy to keep the changed default, and instead provide `WithoutComments()`. I also added tests both ways (and for the existing `WithIndent()` option), and checked that the `comments` tests match the existing ones, and the `default` tests match those from `v2.6.3` (except for the addition of a few descriptions whose omission seem to have been a bug). Comments are still parsed in any case, as adding new struct fields is no problem; and they are still included in `Dump` since that seems obviously parallel to struct fields and is more of a debugging thing.
StevenACoffman
approved these changes
Jul 17, 2023
benjaminjkraft
pushed a commit
to Khan/genqlient
that referenced
this pull request
Jul 17, 2023
See vektah/gqlparser#271 This updates gqlparser dependency to v2.5.8, which defaults to not including comments. This PR will update the snapshots and generated files to match that. The AST will still include the comments in the dump, so the tweak to that one test in parse_test.go is still necessary, so that remains in place. Signed-off-by: Steve Coffman <[email protected]>
mergify bot
referenced
this pull request
in infratographer/metadata-api
Aug 16, 2023
[](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/vektah/gqlparser/v2](https://togithub.com/vektah/gqlparser) | require | patch | `v2.5.6` -> `v2.5.8` | --- ### Release Notes <details> <summary>vektah/gqlparser (github.com/vektah/gqlparser/v2)</summary> ### [`v2.5.8`](https://togithub.com/vektah/gqlparser/releases/tag/v2.5.8) [Compare Source](https://togithub.com/vektah/gqlparser/compare/v2.5.7...v2.5.8) #### What's Changed - Put comments behind an option in formatter by [@​benjaminjkraft](https://togithub.com/benjaminjkraft) in [https://github.com/vektah/gqlparser/pull/271](https://togithub.com/vektah/gqlparser/pull/271) **Full Changelog**: vektah/gqlparser@v2.5.7...v2.5.8 ### [`v2.5.7`](https://togithub.com/vektah/gqlparser/releases/tag/v2.5.7) [Compare Source](https://togithub.com/vektah/gqlparser/compare/v2.5.6...v2.5.7) #### What's Changed - \[Snyk] Security upgrade [@​babel/preset-env](https://togithub.com/babel/preset-env) from 7.16.11 to 7.22.6 by [@​lwc](https://togithub.com/lwc) in [https://github.com/vektah/gqlparser/pull/267](https://togithub.com/vektah/gqlparser/pull/267) - Bump semver from 5.7.1 to 5.7.2 in /validator/imported by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/vektah/gqlparser/pull/268](https://togithub.com/vektah/gqlparser/pull/268) - Allow ommitting Directive arguments that are non-null if they have defaults by [@​StevenACoffman](https://togithub.com/StevenACoffman) in [https://github.com/vektah/gqlparser/pull/270](https://togithub.com/vektah/gqlparser/pull/270) **Full Changelog**: vektah/gqlparser@v2.5.6...v2.5.7 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/infratographer/metadata-api). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNDEuMyIsInVwZGF0ZWRJblZlciI6IjM2LjQzLjIiLCJ0YXJnZXRCcmFuY2giOiJtYWluIn0=--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
github-merge-queue bot
referenced
this pull request
in infratographer/x
Aug 21, 2023
[](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/vektah/gqlparser/v2](https://togithub.com/vektah/gqlparser) | require | patch | `v2.5.6` -> `v2.5.8` | --- ### Release Notes <details> <summary>vektah/gqlparser (github.com/vektah/gqlparser/v2)</summary> ### [`v2.5.8`](https://togithub.com/vektah/gqlparser/releases/tag/v2.5.8) [Compare Source](https://togithub.com/vektah/gqlparser/compare/v2.5.7...v2.5.8) #### What's Changed - Put comments behind an option in formatter by [@​benjaminjkraft](https://togithub.com/benjaminjkraft) in [https://github.com/vektah/gqlparser/pull/271](https://togithub.com/vektah/gqlparser/pull/271) **Full Changelog**: vektah/gqlparser@v2.5.7...v2.5.8 ### [`v2.5.7`](https://togithub.com/vektah/gqlparser/releases/tag/v2.5.7) [Compare Source](https://togithub.com/vektah/gqlparser/compare/v2.5.6...v2.5.7) #### What's Changed - \[Snyk] Security upgrade [@​babel/preset-env](https://togithub.com/babel/preset-env) from 7.16.11 to 7.22.6 by [@​lwc](https://togithub.com/lwc) in [https://github.com/vektah/gqlparser/pull/267](https://togithub.com/vektah/gqlparser/pull/267) - Bump semver from 5.7.1 to 5.7.2 in /validator/imported by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/vektah/gqlparser/pull/268](https://togithub.com/vektah/gqlparser/pull/268) - Allow ommitting Directive arguments that are non-null if they have defaults by [@​StevenACoffman](https://togithub.com/StevenACoffman) in [https://github.com/vektah/gqlparser/pull/270](https://togithub.com/vektah/gqlparser/pull/270) **Full Changelog**: vektah/gqlparser@v2.5.6...v2.5.7 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/infratographer/x). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi41LjMiLCJ1cGRhdGVkSW5WZXIiOiIzNi44LjExIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #263, we introduced parsing of comments, as well as support for them in the formatter. In some cases this is surely useful, but in others it's just bloat. (And as I describe in Khan/genqlient#282, it may even be a problem in some cases which depended on the fact that formatting the query didn't include comments.)
In this commit I introduce an option to control whether comments are formatted. I set the default to false (i.e. restoring the previous behavior if no options are set), because adding this felt to me like a breaking change (so this reverts it), and because it seems to me like the more common usage (which we may as well follow if it's not a breaking change).
WithComments()restores the behavior added in #263. If others disagree I'm happy to keep the changed default, and instead provideWithoutComments(). I also added tests both ways (and for the existingWithIndent()option), and checked that thecommentstests match the existing ones, and thedefaulttests match those fromv2.6.3(except for the addition of a few descriptions whose omission seems to have been a bug).Comments are still parsed in any case, as adding new struct fields is no problem; and they are still included in
Dumpsince that seems obviously parallel to struct fields and is more of a debugging thing.I have: