Add a clear task #40
Add a clear task #40
Conversation
|
Note that without silverstripe/silverstripe-graphql#448 the logger output doesn't get printed on the client. |
|
The task definitely works. I'm not sure about deleting the whole directory for reasons I mentioned inline. |
|
I'd recommend adding some CI integration to perform the linting checks so that reviewers don't have to provide feedback that a linter can provide more consistently |
Co-authored-by: Steve Boyd <emteknetnz@gmail.com>
…rs/silverstripe-graphql-devtools into pulls/master/clear-task
|
Added a CI set up and some basic test for the clear method. |
There was a problem hiding this comment.
The way the test is skipped is consistent with how we handle other v4-only tests in other modules, and the clear controller isn't added to the development admin for use if the same v4 class is missing. I think that's the right way to handle that.
Just a question here about the branch alias being removed - I don't have any context as to why it was there in the first place, but there is definitely no context in the PR for why it is being removed either.
| } | ||
| }, | ||
| "extra": { | ||
| "branch-alias": { | ||
| "dev-master": "1.x-dev" |
There was a problem hiding this comment.
Why is this being removed?
There was a problem hiding this comment.
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.
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.
I don't think this needs a separate PR
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.
My instinct would be to start tagging release numbers for this.
|
Probably should add a line or two in the README about this new functionality as well. |
| "require": { | ||
| "silverstripe/graphql": "*" | ||
| "silverstripe/graphql": "^3 || ^4", |
There was a problem hiding this comment.
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.
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.
Oh right but the rest of this module does work with graphql3? Just not clear?
There was a problem hiding this comment.
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.
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.
we should tag a v3 version and a v4 version to avoid confusion
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.
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
Co-authored-by: Steve Boyd <emteknetnz@gmail.com>
Can you please add this? |
|
I updated the DOC and spun off the other points into another card. #42 |
|
@emteknetnz Do you want any more changes to this? |
This task was put together to run some benchmarking. It still has some value even if it's not really needed in the main module.
I've also added a few tweaks to the composer file.
Parent issue