Skip to content

/ java-storage

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

feat: expose the methods of Notifications #399

Open
wants to merge 16 commits into
base: master
from
Open

feat: expose the methods of Notifications #399

wants to merge 16 commits into from

Conversation

@athakor
Copy link
Member

athakor commented Jun 30, 2020

Addressed the feedback of #393

Fixes #138
@googlebot googlebot added the cla: yes label Jun 30, 2020
@athakor athakor requested review from frankyn and JesseLovelace Jun 30, 2020
@codecov
Copy link

codecov bot commented Jun 30, 2020 •
edited

Codecov Report

Merging #399 into master will increase coverage by 0.32%.
The diff coverage is 74.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #399      +/-   ##
============================================
+ Coverage     63.19%   63.52%   +0.32%     
- Complexity      613      649      +36     
============================================
  Files            32       34       +2     
  Lines          5144     5346     +202     
  Branches        492      501       +9     
============================================
+ Hits           3251     3396     +145     
- Misses         1729     1771      +42     
- Partials        164      179      +15
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/google/cloud/storage/Storage.java 80.64% <ø> (ø) 0.00 <0.00> (ø)
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java 1.58% <0.00%> (-0.02%) 2.00 <0.00> (ø)
...ogle/cloud/storage/spi/v1/HttpStorageRpcSpans.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...va/com/google/cloud/storage/spi/v1/StorageRpc.java 66.23% <ø> (ø) 0.00 <0.00> (ø)
...in/java/com/google/cloud/storage/Notification.java 61.53% <61.53%> (ø) 3.00 <3.00> (?)
...ain/java/com/google/cloud/storage/StorageImpl.java 80.61% <74.35%> (-0.40%) 142.00 <4.00> (+4.00) ⬇️
...ava/com/google/cloud/storage/NotificationInfo.java 85.83% <85.83%> (ø) 28.00 <28.00> (?)
...ogle/cloud/storage/testing/StorageRpcTestBase.java 96.07% <100.00%> (+0.07%) 49.00 <1.00> (+1.00)
...in/java/com/google/cloud/storage/StorageBatch.java 88.00% <0.00%> (-4.00%) 13.00% <0.00%> (ø%)
...main/java/com/google/cloud/storage/BucketInfo.java 81.69% <0.00%> (-0.32%) 86.00% <0.00%> (ø%)
... and 4 more

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 f0e945e...e624bf8. Read the comment docs.
@dmitry-fa
dmitry-fa reviewed Jun 30, 2020
View changes
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jun 30, 2020
View changes
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jun 30, 2020
View changes
...oud-storage/src/main/java/com/google/cloud/storage/NotificationInfo.java Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jun 30, 2020
View changes
...oud-storage/src/main/java/com/google/cloud/storage/NotificationInfo.java Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jun 30, 2020
View changes
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Show resolved Hide resolved
@dmitry-fa
dmitry-fa requested changes Jun 30, 2020
View changes
Copy link
Contributor

dmitry-fa left a comment

please use @param for arguments when you write javadoc for method
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jun 30, 2020
View changes
...ud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java Show resolved Hide resolved
@elharo
elharo requested changes Jul 1, 2020
View changes
...oud-storage/src/main/java/com/google/cloud/storage/NotificationInfo.java Show resolved Hide resolved
@athakor
Copy link
Member Author

athakor commented Jul 2, 2020

@dmitry-fa all the comments have been addressed PTAL
@dmitry-fa
dmitry-fa reviewed Jul 2, 2020
View changes
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Outdated Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jul 2, 2020
View changes
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Outdated Show resolved Hide resolved
@dmitry-fa
dmitry-fa requested changes Jul 2, 2020
View changes
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jul 2, 2020
View changes
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Outdated Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jul 2, 2020
View changes
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jul 2, 2020
View changes
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
@athakor
Copy link
Member Author

athakor commented Jul 6, 2020

@dmitry-fa PTAL
1 similar comment
@athakor
Copy link
Member Author

athakor commented Jul 13, 2020

@dmitry-fa PTAL
@dmitry-fa
dmitry-fa reviewed Jul 13, 2020
View changes
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Show resolved Hide resolved
@dmitry-fa
dmitry-fa reviewed Jul 13, 2020
View changes
Copy link
Contributor

dmitry-fa left a comment

It becomes more and more clear. Some minor things to fix left.
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Show resolved Hide resolved
...e-cloud-storage/src/main/java/com/google/cloud/storage/Notification.java Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
@athakor athakor added the automerge label Jul 13, 2020
@frankyn frankyn removed the automerge label Jul 13, 2020
@googleapis googleapis deleted a comment from gcf-merge-on-green bot Jul 14, 2020
@dmitry-fa
dmitry-fa requested changes Jul 14, 2020
View changes
Copy link
Contributor

dmitry-fa left a comment

a few minor corrections to the doc
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
@athakor
Copy link
Member Author

athakor commented Jul 14, 2020

@dmitry-fa all the comments have been addressed PTAL
@frankyn
Copy link
Member

frankyn commented Jul 23, 2020

Thanks for your patience folks, I want @JesseLovelace to have a look before merging this PR into master.
@frankyn
frankyn requested changes Jul 23, 2020
View changes
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Show resolved Hide resolved
@frankyn
frankyn requested changes Jul 24, 2020
View changes
Copy link
Member

frankyn left a comment

This is better, thanks @athakor. Have two small nits.
...oud-storage/src/main/java/com/google/cloud/storage/NotificationInfo.java Outdated Show resolved Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java Outdated Show resolved Hide resolved
@athakor
Copy link
Member Author

athakor commented Jul 27, 2020

@frankyn @JesseLovelace PTAL
athakor added 16 commits Jun 26, 2020
@athakor
feat: expose bucket notifications apis
Loading status checks…
f0eba7e
@athakor
feat: expose bucket notifications apis
Loading status checks…
470b69c
@athakor
feat: simplified the code and fixed the review changes
Loading status checks…
877efb5
@athakor
feat: fix review changes
Loading status checks…
8a6d578
@athakor
feat: addressed review changes
Loading status checks…
ee3f6dc
@athakor
feat: change javaDoc
Loading status checks…
3e614ca
@athakor
feat: addressed review changes
Loading status checks…
6a88805
@athakor
feat: fix review changes
Loading status checks…
e7fbadc
@athakor
feat: addressed few nits
Loading status checks…
15c90d4
@athakor
feat: fix review changes
Loading status checks…
698fd52
@athakor
feat: fix minor changes and change the tests to provide better coverage
Loading status checks…
54150b1
@athakor
feat: add more checks in test
Loading status checks…
eac6216
@athakor
feat: updated checks
Loading status checks…
f8c8abf
@athakor
feat: address the additional comments
Loading status checks…
36a7ff9
@athakor
feat: addressed the nits
Loading status checks…
c03fc14
@athakor
feat: update test method name
Loading status checks…
e624bf8
@athakor athakor force-pushed the athakor:storage-notification-138 branch from ef93802 to e624bf8 Aug 6, 2020
@athakor
Copy link
Member Author

athakor commented Aug 6, 2020 •
edited

@frankyn @JesseLovelace gentle ping
@athakor
Copy link
Member Author

athakor commented Sep 1, 2020

@frankyn @JesseLovelace friendly ping
@frankyn frankyn requested review from frankyn and elharo Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-fa dmitry-fa

@elharo elharo

@frankyn frankyn

@JesseLovelace JesseLovelace

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Expose the methods of Notifications
6 participants
@athakor @dmitry-fa @frankyn @elharo @googlebot @yoshi-kokoro
You can’t perform that action at this time.