Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Inconsistencies in Consuming REST guide #28

Merged
merged 2 commits into from
Aug 20, 2018
Merged

Inconsistencies in Consuming REST guide #28

merged 2 commits into from
Aug 20, 2018

Conversation

KyleAure
Copy link
Member

The following inconsistencies have been addressed:

  • Should work with whole classes instead of code snippets
  • Instruction asking users to create new classes is inconsistent with the other guides (and within the guide itself too)?
  • Some code blocks are copyable when they shouldn't or don't need to be copyable
  • Do we really need to link to the JSON Binding 1.0 User Guide twice? And, do we need to mention the version?
  • Command blocks should be copyable and be made consistent with the other guides
  • Revise the names of the JUnit tests to be consistent and so we can easily tell which ones are for JSON-B and which is for JSON-P
  • Use mvn compile throughout: Kubik42 mentioned that we should do this. Noting it and Dmitry can elaborate on what it means.
  • Mention under 'Starting the service' that the guide is setup to use loose application/config so users will have the service started from /start and making updates to code directly where the changes will be picked by the server dynamically.

@KyleAure KyleAure self-assigned this Jul 24, 2018
@evelinec evelinec requested a review from Kubik42 July 24, 2018 19:37
KyleAure
KyleAure commented Aug 15, 2018
View reviewed changes
README.adoc

[source, java, indent=0]
----
include::finish/src/main/java/io/openliberty/guides/consumingrest/Consumer.java[tags=!comment;!class]
include::finish/src/main/java/io/openliberty/guides/consumingrest/Consumer.java[tags=!comment;]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evelinec Combined the code snippets into this one area as instructed by issue here: #23

evelinec
evelinec reviewed Aug 15, 2018
View reviewed changes
README.adoc Outdated
@@ -1,4 +1,4 @@
// Copyright (c) 2017 IBM Corporation and others.
// Copyright (c) 2018 IBM Corporation and others.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're updating an existing file, the copyright year should be 2017, 2018, you can just change it to 2018.

README.adoc Outdated

`src/main/java/io/openliberty/guides/consumingrest/Consumer.java`:
The `Artist` and `Album` POJOs are ready for deserialization. To consume the JSON response
from your REST service create the `Consumer` class in the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a comma is needed in "from your REST service, create the Consumer class in the"

README.adoc Outdated
`src/main/java/io/openliberty/guides/consumingrest/Consumer.java`:
The `Artist` and `Album` POJOs are ready for deserialization. To consume the JSON response
from your REST service create the `Consumer` class in the
`src/main/java/io/openliberty/guides/consumingrest/Consumer.java` file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a ":" after file instead of "."

README.adoc Outdated
the `start` directory.

`src/main/java/io/openliberty/guides/consumingrest/service/ArtistResource.java`:
the `src/main/java/io/openliberty/guides/consumingrest/service/ArtistResource.java` file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a ":" after file instead of "."

README.adoc Outdated

// =================================================================================================
// Testing deserialization
// =================================================================================================

== Testing deserialization

Create a test class, `src/test/java/it/io/openliberty/guides/consumingrest/ConsumingRestTest.java`.
Create the `ConsumingRestTest` class in the
`src/test/java/it/io/openliberty/guides/consumingrest/ConsumingRestTest.java` file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a ":" after file instead of "."

finish/src/main/java/io/openliberty/guides/consumingrest/Consumer.java Outdated
@@ -1,6 +1,6 @@
// tag::comment[]
/*******************************************************************************
* Copyright (c) 2017 IBM Corporation and others.
* Copyright (c) 2018 IBM Corporation and others.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for the copyright year.

@evelinec evelinec mentioned this pull request Aug 16, 2018
Closed
5 tasks
@evelinec
Copy link
Contributor

I'm okay with the change needed for issue #32.

@evelinec
Copy link
Contributor

@Kubik42 can you review the other changes in this PR?

Kubik42
Kubik42 suggested changes Aug 17, 2018
View reviewed changes
Copy link
Contributor

@Kubik42 Kubik42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few things, looks good otherwise.

README.adoc Outdated
=== Processing JSON using JSON-B

JSON-B is a Java API that is used to serialize Java objects to JSON messages and vice versa.

To include the JSON-B provider in your project, add the following dependency to your `pom.xml`
Open Liberty's `jsonb-1.0` feature on Maven Central includes the JSON-B provider through transitive dependencies. To include the JSON-B provider in your project, add the following dependency to your `pom.xml`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid listing the feature version as this doc will have to be updated everytime a new version of the feature is released. Perhaps we can say "Open Liberty's JSON-B feature..."?

@KyleAure
Copy link
Member Author

KyleAure commented Aug 17, 2018 •
edited
Loading

@Kubik42 I actually did not write that part but it has been changed! 😆

@Kubik42
Copy link
Contributor

Kubik42 commented Aug 20, 2018

I'm fixing my own mistakes then :P Looks ok to merge to me, thanks for all your work on this one @KyleAure

@KyleAure KyleAure merged commit 9733d7b into master Aug 20, 2018
@evelinec evelinec deleted the 23-Inconsistencies branch February 26, 2019 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evelinec evelinec evelinec left review comments

@Kubik42 Kubik42 Kubik42 approved these changes

Assignees

@KyleAure KyleAure

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@KyleAure @evelinec @Kubik42