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

feat: add file.isPublic() function #708

Merged
merged 15 commits into from
May 30, 2019
Merged

Conversation

AVaksman
Copy link
Contributor

Fixes #683

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 10, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 10, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 10, 2019
@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #708 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
+ Coverage   96.77%   96.79%   +0.01%     
==========================================
  Files           9        9              
  Lines        1149     1155       +6     
  Branches      294      295       +1     
==========================================
+ Hits         1112     1118       +6     
  Misses          9        9              
  Partials       28       28
Impacted Files Coverage Δ
src/file.ts 98.12% <100%> (+0.02%) ⬆️

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 b2f8a74...06474dc. Read the comment docs.

@AVaksman AVaksman marked this pull request as ready for review May 10, 2019 14:24
Copy link
Member

@jkwlui jkwlui left a comment

Choose a reason for hiding this comment

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

gaxios is not in package.json explicitly. If we're using gaxios to make requests, then we should add it to package.json under dependencies

@AVaksman
Copy link
Contributor Author

gaxios is not in package.json explicitly. If we're using gaxios to make requests, then we should add it to package.json under dependencies

Done, thank you

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 14, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 14, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 14, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 14, 2019
test/file.ts Outdated Show resolved Hide resolved
src/file.ts Outdated
isPublic(callback?: IsPublicCallback): Promise<IsPublicResponse> | void {
gaxiosRequest({
method: 'HEAD',
url: `http://${this.bucket.name}.storage.googleapis.com/${this.id}`,
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 we need to encode the name:

url: `http://${this.bucket.name}.storage.googleapis.com/${encodeURIComponent(this.name)}`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, done

It only matters in the case where file.name property is changed on an already existing file object, which would render all other file properties' values incorrect in respect to the new file.name.
Shouldn't the file.name property be made readonly?

Copy link
Contributor

Choose a reason for hiding this comment

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

It only matters in the case where file.name property is changed

Do you mean the name property is already encoded? I didn't see that in the code.

Shouldn't the file.name property be made readonly?

I suppose. name wasn't intended to be a public property at all.

test/file.ts Show resolved Hide resolved
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label May 17, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 20, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 20, 2019
@bcoe bcoe changed the title feature: add file.isPublic() function feat: add file.isPublic() function May 21, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 24, 2019
Copy link
Contributor

@stephenplusplus stephenplusplus left a comment

Choose a reason for hiding this comment

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

Sorry I didn't think of this sooner.

src/file.ts Outdated
* @property {boolean} 0 Whether file is public or not.
*/
/**
* Check whether this file is public or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should explain how we are doing this. It is possible the server has a hiccup and returns an error code, even though the file is public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!
Thanks

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 29, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 29, 2019
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Thanks @AVaksman for putting this together. Apologies for the delay in my review, LGTM.

@AVaksman
Copy link
Contributor Author

Thanks @AVaksman for putting this together. Apologies for the delay in my review, LGTM.

Thanks.

@AVaksman AVaksman merged commit f86cadb into googleapis:master May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File method to check whether an Object is Public
9 participants