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.

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

Implement ack and nack methods, add javadoc and tests #1027

Merged
merged 1 commit into from
Jun 1, 2016

Conversation

mziccard
Copy link
Contributor

This PR implements ack and nack methods, adds javadoc and unit tests. System tests will come as soon as pull methods are implemented.

@mziccard mziccard added the api: pubsub Issues related to the Pub/Sub API. label May 26, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 26, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 84.374% when pulling 264755c on mziccard:pubsub-ack-nack into b872399 on GoogleCloudPlatform:pubsub-alpha.

* acknowledge, as returned in {@link ReceivedMessage#ackId()} by {@link #pull(String, int)} and
* {@link #pullAsync(String, int)}.
*
* @param subscription the subscription whose messages must be acknowledged

This comment was marked as spam.

This comment was marked as spam.

@lesv
Copy link
Contributor

lesv commented May 27, 2016

Once I understand (or get feedback from JJ or Arie) the javadoc story for the repo, I expect to give quite a bit of feedback.

The code and the testing look ok, but I want to spend a few minutes at an earlier time of day re-reviewing to make sure that I believe your tests actually cover things and are helpful.

@aozarov
Copy link
Contributor

aozarov commented May 27, 2016

I think so far we were careful to follow the correct Javadoc guidlines for structure (spaces, tag ordering) and rules (punctuation, case,..). As for the descriptions, I think the focus was on clarity which in some
cases made it less concise. You can find in both the JDK and Guava examples to match either style.
I don't have strong opinion about it, however if you think clarity is not effected or even improved by making it shorter than I think you should go for it.

BTW, Looking at the generated Javadoc it looks like @param descriptions are not enabled.

@mziccard
Copy link
Contributor Author

BTW, Looking at the generated Javadoc it looks like @param descriptions are not enabled.

Arie, what do you mean with this?

@aozarov
Copy link
Contributor

aozarov commented May 27, 2016

Arie, what do you mean with this?

I was wrong. I sampled few places (just to see how things look like per earlier discussion) and unfortunately each of these did not specify @param...
E.g. this and this and this and more...

@lesv
Copy link
Contributor

lesv commented May 28, 2016

Our internal guidelines specify that it should be a fragment, not a sentence and that it's ok to not use @param if you mention the value in the paragraph. It also specifies fewer words (ie. fragments) are better as long as it's clear.

So, I'm going to suggest a few re-wordings to be simpler - we won't go through and change existing stuff, but I think being concise is better for our users.

@mziccard
Copy link
Contributor Author

Not to be picky but let me clarify. Fragments are sequences of words that look like a sentence but are not. The difference from a sentence is that fragments are allowed to contain no independent clauses. An independent clause is a sequence of words that contains a subject and a verb and makes sense by itself (extrapolated from the context).

Acknowledges the given messages for the provided subscription. is already a fragment (it's missing the subject). @lesv suggestion (Acknowledges given messages for provided subscription.) simply removes the articles but does not affect its fragment nature.

For what concerns @param descriptions, according to Oracle's guide, they should start with a noun (hence the suggested @param subscription whose messages are being acknowledged is wrong), possibly preceded by an article.

I don't mind applying changes (even project-wide) if that improve docs readability I just doubt that removing some articles or conjunctions would help users.

@lesv
Copy link
Contributor

lesv commented May 31, 2016

Yep - I agree - It's what I've been pondering. I do dislike several mentions of the same thing for the future. LGTM

@mziccard
Copy link
Contributor Author

mziccard commented Jun 1, 2016

Thanks for the pass @lesv

@mziccard mziccard merged commit 6df07ba into googleapis:pubsub-alpha Jun 1, 2016
mziccard added a commit to mziccard/gcloud-java that referenced this pull request Jun 27, 2016
github-actions bot pushed a commit that referenced this pull request Oct 5, 2022
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
github-actions bot pushed a commit that referenced this pull request Oct 5, 2022
github-actions bot pushed a commit to renovate-bot/google-cloud-java that referenced this pull request Oct 5, 2022
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
github-actions bot pushed a commit to yoshi-code-bot/google-cloud-java that referenced this pull request Nov 8, 2022
…s#1687) (googleapis#1027)

* chore(java): add a note in README for migrated split repos

Disable renovate bot and flaky bot for split repositories
that have moved to the Java monorepo.
The Java monorepo will pass the "monorepo=True" parameter
to java.common_templates method in its owlbot.py files so that
the migration note will not appear in the README in the monorepo.

Co-authored-by: Jeff Ching <chingor@google.com>
Source-Link: https://github.com/googleapis/synthtool/commit/d4b291604f148cde065838c498bc8aa79b8dc10e
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:edae91ccdd2dded2f572ec341a768ad180305a3e8fbfd93064b28e237d35920a
suztomo pushed a commit that referenced this pull request Feb 1, 2023
Source-Link: googleapis/synthtool@fbc8bfe
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:e76136cc48f90aa19ba29cdfbd4002111467e44a1c9d905867d98dafafbd03bb

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants