-
-
Notifications
You must be signed in to change notification settings - Fork 339
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
Improve transaction list for GraphQL requests #805
Conversation
Great library people, hope this is useful and we can release it soon |
Ohh @cortinico Thanks! I saw #70 because I come from the issue, but didn't spot #800 when I start to do these changes! I'm happy to move with 800 if @alkber wanna fix the issues open there. I'm happy to close this one. From another point of view, my implementation is a simple one and since the tests passed(if the code is valid) we could merge and use the 800 as an iterative improvement. I see that my sample implementation differs from the other PR too because I'm using a real schema from the guides, would be interesting to know what will be the best solution. That said, any of the solutions work for me if we can put them to work =D. Let me know @alkber |
Hi @cortinico , isorry i was really busy with personal work and couldn't address the comments in PR #800. I believe @Canato approach is more generic and neat than mine as i read the code, and he has decorated the PR with more test cases too.. You may close PR #800 and use this one. However I have a small suggestion, to @Canato PR. If the UI item can be updated to the way PR #800 does then it would be much appreciated and visually appealing. @Canato can you take it up? you can reuse my work in your code. |
If @cortinico is ok I can update the UI to use the graphql icon and add a new line ^^. Thanks @alkber |
@cortinico news on this? =D |
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.
Great stuff! Thanks for doing this. Sorry for the late review 🙏
Let me know if you need some help integrating the changes I suggested
downloadApolloSchema.sh
Outdated
@@ -0,0 +1,9 @@ | |||
./gradlew downloadApolloSchema --endpoint="https://rickandmortyapi.com/graphql" --schema="sample/src/main/graphql/com/chuckerteam/chucker/sample/schema.json" |
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.
Please add a #!/bin/bash
on top
library/src/main/kotlin/com/chuckerteam/chucker/internal/data/entity/HttpTransactionTuple.kt
Outdated
Show resolved
Hide resolved
library/src/main/kotlin/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt
Show resolved
Hide resolved
@@ -53,7 +53,8 @@ internal class HttpTransaction( | |||
@ColumnInfo(name = "responseHeadersSize") var responseHeadersSize: Long?, | |||
@ColumnInfo(name = "responseBody") var responseBody: String?, | |||
@ColumnInfo(name = "isResponseBodyEncoded") var isResponseBodyEncoded: Boolean = false, | |||
@ColumnInfo(name = "responseImageData") var responseImageData: ByteArray? | |||
@ColumnInfo(name = "responseImageData") var responseImageData: ByteArray?, | |||
@ColumnInfo(name = "graphQlOperationName") var graphQlOperationName: String?, |
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.
Just curious, but wouldn't we need to include a check for this parameter in the fun hasTheSameContent()
?
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.
Good call, I've added it 👍
Any news on this? Worth to solve the conflicts ? |
Yes, worth solving. Let's make it happen. |
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've rebased your changes @Canato and fixed the detekt failures 👍 This is good to merge on my end.
Thanks @cortinico how do we know when will be the next release or if this will be available in a snapshot? |
No sorry we don't have an ETA at the moment. I'm looking into wrapping up the work for a 4.x. |
No issues, Thanks! =D |
Close #69
Close #116
🎥 Video
SVID_20220421_011146_1.mp4
📷 Image
after iterative improvement
📄 Context
Based on #69, #116 and a little on #579 we have a desire for a more clean list when using GraphQL.
This helps to debug who uses Graphql without changing normal okhttp requests
📝 Changes
Retrieve request header for transaction list and display it if is a GraphQl request and have the
operationName
🛠️ How to test
On the Sample code, there is a new button
Do GraphQl Activity
that can be pressed for testing.⏱️ Next steps
Wanna be sure the code I add is following the library standards, open to suggestions!