-
Notifications
You must be signed in to change notification settings - Fork 13
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
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 <[email protected]>
…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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
travis builds are failing
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 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.
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 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.
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.
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.
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.
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.
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
Co-authored-by: Steve Boyd <[email protected]>
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