jamesfredley commented on code in PR #520:
URL: 
https://github.com/apache/grails-static-website/pull/520#discussion_r3590404597


##########
guides/grails-http-client/v8/guide/httpClientSetup.adoc:
##########
@@ -0,0 +1,39 @@
+Register HTTP service interfaces on the application class with 
`@ImportHttpServices`:
+
+[source,groovy]
+.grails-app/init/example/Application.groovy
+----
+include::../snippets/grails-app/init/example/Application.groovy[]
+----
+
+Spring Boot scans `example` for `@HttpExchange` interfaces and auto-configures 
a `RestClient` proxy for each one.

Review Comment:
   This is inaccurate about the mechanism. It is not Spring Boot component 
scanning / autoconfiguration that registers the client - it's 
`@ImportHttpServices(basePackages = 'example')` (Spring Framework 7 / Spring 
Boot 4, which Grails 8 ships) that scans `example` for `@HttpExchange` 
interfaces and registers an HTTP-service proxy for each. Suggested reword:
   
   > `@ImportHttpServices` scans `example` for `@HttpExchange` interfaces and 
registers a `RestClient`-backed proxy bean for each one.
   
   Worth noting this requires the Grails 8 Spring 7 / Boot 4 baseline.



##########
guides/grails-http-client/v8/guide/gettingStarted.adoc:
##########
@@ -0,0 +1,22 @@
+Clone the repository and run the starting application:
+
+[source,bash]
+----
+git clone -b grails8 https://github.com/grails-guides/grails-http-client.git
+cd grails-http-client/initial
+./gradlew bootRun
+----
+
+Open http://localhost:8080/[http://localhost:8080/] for the welcome JSON 
payload. The `initial/` project is a vanilla Grails 8 REST API starter with no 
HTTP client yet.
+
+To skip ahead, `cd complete` and run the same commands — that tree contains 
the finished `@HttpExchange` client, services, and tests.

Review Comment:
   Two issues on this line:
   
   1. **Broken `cd`** - the reader was just told to `cd 
grails-http-client/initial` above, so `cd complete` resolves to 
`initial/complete` (which doesn't exist). Use `cd ../complete`.
   2. **Em dash** - `commands — that tree` should use a plain hyphen `-` per 
the repo's typography convention.



##########
guides/grails-http-client/v8/guide/requirements.adoc:
##########
@@ -0,0 +1,5 @@
+* Approximately 45 minutes
+* JDK 21 (Apache Grails 8 requires Java 21)
+* A https://www.grails.org/[Grails] installation or the bundled Gradle wrapper 
in `initial/` and `complete/`
+* **Docker** — required for integration tests (Testcontainers PostgreSQL)
+* **PostgreSQL** on `localhost:5432` — required for `./gradlew bootRun` in 
both `initial/` and `complete/` (default database `devDb`; see 
`grails-app/conf/application.yml`)

Review Comment:
   **Em dash** - `localhost:5432` — required` should be a plain hyphen `-`.



##########
guides/grails-http-client/v8/guide/testing.adoc:
##########
@@ -0,0 +1,52 @@
+[[unitTests]]
+== Unit tests
+
+Domain constraints:
+
+[source,groovy]
+.src/test/groovy/example/RecordLabelSpec.groovy
+----
+include::../snippets/src/test/groovy/example/RecordLabelSpec.groovy[]
+----
+
+Service delegation to the HTTP client with a mock:
+
+[source,groovy]
+.src/test/groovy/example/ItunesSearchServiceSpec.groovy
+----
+include::../snippets/src/test/groovy/example/ItunesSearchServiceSpec.groovy[]
+----
+
+[[integrationTests]]
+== Integration tests
+
+Verify the HTTP client is registered as a Spring bean:
+
+[source,groovy]
+.src/integration-test/groovy/example/ItunesClientIntegrationSpec.groovy
+----
+include::../snippets/src/integration-test/groovy/example/ItunesClientIntegrationSpec.groovy[]
+----
+
+Assert GORM persistence against real PostgreSQL (Testcontainers):
+
+[source,groovy]
+.src/integration-test/groovy/example/RecordLabelIntegrationSpec.groovy
+----
+include::../snippets/src/integration-test/groovy/example/RecordLabelIntegrationSpec.groovy[]
+----
+
+Run unit tests:
+
+[source,bash]
+----
+cd complete

Review Comment:
   `cd complete` assumes the reader is at the repository root, but following 
the guide they were left in `grails-http-client/initial` (see gettingStarted). 
Either use `cd ../complete` or state explicitly that this block runs from the 
repo root.



##########
guides/grails-http-client/v8/guide/requirements.adoc:
##########
@@ -0,0 +1,5 @@
+* Approximately 45 minutes
+* JDK 21 (Apache Grails 8 requires Java 21)
+* A https://www.grails.org/[Grails] installation or the bundled Gradle wrapper 
in `initial/` and `complete/`
+* **Docker** — required for integration tests (Testcontainers PostgreSQL)

Review Comment:
   **Em dash** - `**Docker** — required` should be a plain hyphen `-`.



##########
guides/grails-http-client/v8/guide/httpClientSetup.adoc:
##########
@@ -0,0 +1,39 @@
+Register HTTP service interfaces on the application class with 
`@ImportHttpServices`:
+
+[source,groovy]
+.grails-app/init/example/Application.groovy
+----
+include::../snippets/grails-app/init/example/Application.groovy[]
+----
+
+Spring Boot scans `example` for `@HttpExchange` interfaces and auto-configures 
a `RestClient` proxy for each one.
+
+[[itunesClient]]
+== ItunesClient
+
+Declare the iTunes Search API client as a Spring HTTP service interface:
+
+[source,groovy]
+.src/main/groovy/example/ItunesClient.groovy
+----
+include::../snippets/src/main/groovy/example/ItunesClient.groovy[]
+----
+
+[[dtos]]
+== Response DTOs
+
+Map the JSON response with simple POGOs:
+
+[source,groovy]
+.src/main/groovy/example/Album.groovy
+----
+include::../snippets/src/main/groovy/example/Album.groovy[]
+----
+
+[source,groovy]
+.src/main/groovy/example/SearchResult.groovy
+----
+include::../snippets/src/main/groovy/example/SearchResult.groovy[]
+----
+
+Spring deserializes the iTunes JSON into these types automatically.

Review Comment:
   This "deserializes ... automatically" claim only holds once a JSON message 
converter is on the classpath. As submitted, the sample app's 
`complete/build.gradle` pulls in neither Jackson nor Gson for the `RestClient` 
(`grails-views-gson` renders views, it is not the client converter), so this 
deserialization currently fails at runtime. Tracked as a blocker on 
grails-guides/grails-http-client#1 (add `spring-boot-starter-json`). Keep this 
sentence, but it depends on that fix landing.



##########
guides/grails-http-client/v8/guide/introduction.adoc:
##########
@@ -0,0 +1,5 @@
+Learn how to call external REST APIs from a Grails 8 application using Spring 
Boot's built-in HTTP Services support: define a `@HttpExchange` interface, 
register it with `@ImportHttpServices`, inject the generated client into a 
Grails service, and expose results through JSON views. Local `RecordLabel` data 
stays in GORM/PostgreSQL; album metadata comes from the iTunes Search API.
+
+No Micronaut plugin or extra HTTP client dependency is required — this uses 
the same Spring stack Grails 8 already runs on.

Review Comment:
   **Em dash** - `required — this uses` should be a plain hyphen `-` per the 
repo's typography convention.



##########
guides/grails-http-client/v8/guide/runningTheApp.adoc:
##########
@@ -0,0 +1,15 @@
+[source,bash]
+----
+cd complete

Review Comment:
   Same `cd complete` cwd assumption as in testing.adoc - after the 
getting-started steps the reader is in `grails-http-client/initial`, so this 
would be `initial/complete`. Use `cd ../complete` or note this block assumes 
the repo root.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to