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.
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
Add a clear task #40
Add a clear task #40
Changes from 11 commits
0505e7f
ec9312f
a396385
4487251
176f50e
9aa6b7f
5f2bf2b
5a63b45
b946f56
1c4a0b6
ee340ce
9f73902
5b11725
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does this actually work with graphql 3? I'm seeing imports in other files such as
SilverStripe\GraphQL\Schema\Logger
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.
The clear functionality doesn't work with v3 - but it also isn't available with v3 (see yml config)
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 right but the rest of this module does work with graphql3? Just not clear?
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.
I believe it does - it at least used to and is supposed to, and I don't think anything in PR would stop it from doing so.
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.
It's meant to work with both of them. However, I'm thinking we should tag a v3 version and a v4 version to avoid confusion. I thought of doing it in this PR, but the travis build wanted to test both of them any way, so it was failing the build.
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.
If it's meant to have dual support, then why not just let it have dual support? Especially since at this point it will be more work to unpick it.
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.
Yeah, the dual support is a bit awful, though pragmatically we should just leave as is. Perhaps do a new major at CMS 5 time that is graphql 4 only
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.
Why is this being removed?
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.
There a history of these things being in Silverstripe modules, they're generally out of date so more annoying than helpful. These days we remove these things any time we see them. I don't think this needs a separate PR.
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 there's no
1
branch and no stable tags. I guess if someone out there is requiring this as ^1 then that'll break.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.
I agree it's okay in this PR so long as we can say why it's happening, and if it should happen. Which you've now provided! Thanks.
In this case I'd say it's okay. We're not even tagging this module, so anyone with
^1
as a constraint with this module imo has the wrong constraint, and it should break for them, as a sanity check.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.
My instinct would be to start tagging release numbers for this.