add support for variables #23

zeitlinger · 2018-05-31T14:53:12Z

This fixes #14

codecov-io · 2018-05-31T14:55:36Z

Codecov Report

Merging #23 into master will increase coverage by 1.75%.
The diff coverage is 91.66%.

@@             Coverage Diff              @@
##             master      #23      +/-   ##
============================================
+ Coverage     82.01%   83.76%   +1.75%     
- Complexity       55       68      +13     
============================================
  Files            15       16       +1     
  Lines           139      154      +15     
  Branches         25       28       +3     
============================================
+ Hits            114      129      +15     
+ Misses           10        9       -1     
- Partials         15       16       +1

Impacted Files	Coverage Δ	Complexity Δ
.../main/kotlin/me/lazmaid/kraph/types/CustomTypes.kt	`100% <100%> (ø)`	`0 <0> (?)`
...src/main/kotlin/me/lazmaid/kraph/lang/Variables.kt	`100% <100%> (ø)`	`9 <9> (+7)`	⬆️
...src/main/kotlin/me/lazmaid/kraph/lang/DataEntry.kt	`100% <100%> (ø)`	`1 <0> (ø)`	⬇️
.../src/main/kotlin/me/lazmaid/kraph/lang/Document.kt	`100% <100%> (ø)`	`5 <1> (+2)`	⬆️
...c/main/kotlin/me/lazmaid/kraph/lang/GraphQLNode.kt	`73.07% <66.66%> (+2.24%)`	`18 <0> (+2)`	⬆️
core/src/main/kotlin/me/lazmaid/kraph/Kraph.kt	`85.18% <83.33%> (+0.87%)`	`12 <2> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3e7284...1dacbe1. Read the comment docs.

VerachadW

Hi, Thanks for the contributions. I put a few comments here. Please let me know if you have any questions.

VerachadW · 2018-06-01T05:46:33Z

core/src/main/kotlin/me/lazmaid/kraph/lang/Variables.kt

-// TODO: implement variables
-internal class Variables() {
-    fun print(): String? = null
+internal class Variables {


All the node should implement GraphQLNode. So, it would be easier to to handle formatting logic

I think it would be confusing if Variables implements GraphQLNode, since the parameters of print (format: PrintFormat, previousLevel: Int) don't make any sense.

At this moment, It may not make sense much to make it implement GraphQLNode, but it will make it more easier if we need to implement some features that apply to all the node in the Kraph

VerachadW · 2018-06-01T05:48:50Z

core/src/main/kotlin/me/lazmaid/kraph/lang/Variables.kt

+                it.entries.joinToString(separator = ",", prefix = "{", postfix = "}") { "\"${it.key}\": ${it.value.jsonValue}" }
+            }
+
+    fun asArgument(): Argument? =


IMO, this should be an extension of this class more than a member function itself.

I don't really care.
It would make some classes (such as Variables) not internal - do you mind?

I think there is no different in this case. You may leave it like this.

VerachadW · 2018-06-01T05:50:57Z

core/src/main/kotlin/me/lazmaid/kraph/Kraph.kt

@@ -11,22 +11,29 @@ typealias FieldBlock = Kraph.FieldBuilder.() -> Unit
 typealias CursorBlock = Kraph.CursorSelectionBuilder.() -> Unit
 typealias NodeBlock = Kraph.NodeBuilder.() -> Unit

+data class KraphVariableType(val value: String)
+data class KraphVariable(val name: String, val type: KraphVariableType, val jsonValue: String) {


I think these 2 classes deserve their own package. Maybe we can have the package for custom GraphQL type. What do you think?

You mean a package that has a class with only those 2 lines of code?
Or any other class as well?

For now, It would be another package with a file contain these 2 classes

VerachadW

Sorry for the delay answers, I really busy for the past week.

VerachadW · 2018-06-11T03:47:14Z

core/src/main/kotlin/me/lazmaid/kraph/lang/Variables.kt

-// TODO: implement variables
-internal class Variables() {
-    fun print(): String? = null
+internal class Variables {


At this moment, It may not make sense much to make it implement GraphQLNode, but it will make it more easier if we need to implement some features that apply to all the node in the Kraph

VerachadW · 2018-06-11T03:48:18Z

core/src/main/kotlin/me/lazmaid/kraph/lang/Variables.kt

+                it.entries.joinToString(separator = ",", prefix = "{", postfix = "}") { "\"${it.key}\": ${it.value.jsonValue}" }
+            }
+
+    fun asArgument(): Argument? =


I think there is no different in this case. You may leave it like this.

VerachadW · 2018-06-11T03:51:06Z

core/src/main/kotlin/me/lazmaid/kraph/Kraph.kt

@@ -11,22 +11,29 @@ typealias FieldBlock = Kraph.FieldBuilder.() -> Unit
 typealias CursorBlock = Kraph.CursorSelectionBuilder.() -> Unit
 typealias NodeBlock = Kraph.NodeBuilder.() -> Unit

+data class KraphVariableType(val value: String)
+data class KraphVariable(val name: String, val type: KraphVariableType, val jsonValue: String) {


For now, It would be another package with a file contain these 2 classes

zeitlinger · 2018-06-12T12:52:55Z

No problem - let me know if it's fine now.

VerachadW

LGTM. Thanks again for your contributions. 👍

* Fix escaped string array (#6) * Update Spek version * Fix print wrong escaped character, Fix #4 * Rewrite the GraphQLPrintSpek for preetty print test * Bump hotfix version * Update build status link * Migrate kotlin v1.1.1 (#7) * bump to kotlin v1.1.1 * Add type alias for more readable code * Change the package name to me.lazmaid.kraph * Fix build failed dur to hamkrest error * Update spek version * Update ENV for release * Update README * Refactor arguments (#9) * WIP * Split the print logic into sealed class * Update UTs * Add exception message * Refactor * Add Test Coverage (#11) * Add Test Coverage * Update spek version * Fix test result path * Add codecov YAML * Add onlyIf * Update config * Fix travis command * Add coverage badge * Fix wrong download badge * Bump version to 0.5.1 * Support boolean type in Argument (#15) * Add support for boolean type * Add Tests * Fix typo in README (#16) missing "h" in the example call to `println(query.toGrapQueryString())` * Added basic fragment support (#17) * improve readme * Allow using field with a block in place of field object * Update readme to include new changes * Cheap implementation of fragment that doesn't use actual GraphQL Fragments * Add fragment docs to readme * Fix tests * Change function to 'defineFragment' * Update Kotlin and library dependcies (#18) * Update .gitignore * Update dependecies * Add secondary constructor for DataEntry.NonDecimalNUmber * Add Test for DataEntry classes * Fix failed UTs * Added partial request printing functions (#19) * improve readme * Allow using field with a block in place of field object * Update readme to include new changes * Cheap implementation of fragment that doesn't use actual GraphQL Fragments * Add fragment docs to readme * Fix tests * Add separate methods to print the different parts of the request separately * Use spaces instead of newlines for request format * Rewrite printing operations to optionally escape quotes * update readme a bit * Change function to 'defineFragment' * Use an enum to choose print format and write tests to cover all formats * Add note about variables * Update version (#20) * add support for variables (#23) * add support for variables * add support for variables * Added enums support * add some test for enums support * add test for GraphQLPrintSpek

* Fix escaped string array (#6) * Update Spek version * Fix print wrong escaped character, Fix #4 * Rewrite the GraphQLPrintSpek for preetty print test * Bump hotfix version * Update build status link * Migrate kotlin v1.1.1 (#7) * bump to kotlin v1.1.1 * Add type alias for more readable code * Change the package name to me.lazmaid.kraph * Fix build failed dur to hamkrest error * Update spek version * Update ENV for release * Update README * Refactor arguments (#9) * WIP * Split the print logic into sealed class * Update UTs * Add exception message * Refactor * Add Test Coverage (#11) * Add Test Coverage * Update spek version * Fix test result path * Add codecov YAML * Add onlyIf * Update config * Fix travis command * Add coverage badge * Fix wrong download badge * Bump version to 0.5.1 * Support boolean type in Argument (#15) * Add support for boolean type * Add Tests * Fix typo in README (#16) missing "h" in the example call to `println(query.toGrapQueryString())` * Added basic fragment support (#17) * improve readme * Allow using field with a block in place of field object * Update readme to include new changes * Cheap implementation of fragment that doesn't use actual GraphQL Fragments * Add fragment docs to readme * Fix tests * Change function to 'defineFragment' * Update Kotlin and library dependcies (#18) * Update .gitignore * Update dependecies * Add secondary constructor for DataEntry.NonDecimalNUmber * Add Test for DataEntry classes * Fix failed UTs * Added partial request printing functions (#19) * improve readme * Allow using field with a block in place of field object * Update readme to include new changes * Cheap implementation of fragment that doesn't use actual GraphQL Fragments * Add fragment docs to readme * Fix tests * Add separate methods to print the different parts of the request separately * Use spaces instead of newlines for request format * Rewrite printing operations to optionally escape quotes * update readme a bit * Change function to 'defineFragment' * Use an enum to choose print format and write tests to cover all formats * Add note about variables * Update version (#20) * add support for variables (#23) * add support for variables * add support for variables * Added alias support for queries and mutations * Checked errors from CI Build and added an example for usage * Resolve error with arguments in RequestSpek.kt * Resolve error in BuilderSpek.kt * Replace /n on """ in the BuilderSpek.kt * Change equalTo to requestString * Change other test mutation with alias * Added documentation about alias for field and fieldObject * Resolved conflicts * Check CI error

add support for variables

1f75c09

VerachadW reviewed Jun 1, 2018

View reviewed changes

VerachadW reviewed Jun 11, 2018

View reviewed changes

add support for variables

1dacbe1

VerachadW approved these changes Jun 13, 2018

View reviewed changes

VerachadW merged commit 4d64d7d into VerachadW:master Jun 13, 2018

jershell mentioned this pull request May 27, 2019

When will the next release? #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for variables #23

add support for variables #23

zeitlinger commented May 31, 2018

codecov-io commented May 31, 2018 •

edited

Loading

VerachadW left a comment

VerachadW Jun 1, 2018

zeitlinger Jun 4, 2018

VerachadW Jun 11, 2018

VerachadW Jun 1, 2018

zeitlinger Jun 4, 2018

VerachadW Jun 11, 2018

VerachadW Jun 1, 2018

zeitlinger Jun 4, 2018

VerachadW Jun 11, 2018

VerachadW left a comment

VerachadW Jun 11, 2018

VerachadW Jun 11, 2018

VerachadW Jun 11, 2018

zeitlinger commented Jun 12, 2018

VerachadW left a comment

add support for variables #23

add support for variables #23

Conversation

zeitlinger commented May 31, 2018

codecov-io commented May 31, 2018 • edited Loading

Codecov Report

VerachadW left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VerachadW left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeitlinger commented Jun 12, 2018

VerachadW left a comment

Choose a reason for hiding this comment

codecov-io commented May 31, 2018 •

edited

Loading