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

Specify folder parameter for folder-based filter #361

Closed
wants to merge 1 commit into from
Closed

Specify folder parameter for folder-based filter #361

wants to merge 1 commit into from

Conversation

ryanseys
Copy link
Contributor

Add folder-based filtering to Bucket#getFiles with a folder query parameter.

Also implemented weird idea for returning prefixes for #342. I'm not sure how bad an idea it is, but basically return the prefixes as a property of the files Array. Yes, a property on an Array... How bad of an idea is that? 👶

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 23, 2015
@ryanseys ryanseys added enhancement api: storage Issues related to the Cloud Storage API. labels Jan 23, 2015
@ryanseys
Copy link
Contributor Author

Agh, tags. What ones do I use?? I just added a boat-load of them.

@@ -235,6 +237,11 @@ Bucket.prototype.file = function(name) {
* bucket.getFiles(nextQuery, function(err, files, nextQuery) {});
* }
*
* // The prefixes property contains any prefixes returned in the response.

This comment was marked as spam.

This comment was marked as spam.

@stephenplusplus
Copy link
Contributor

Agh, tags. What ones do I use?? I just added a boat-load of them.

Lol

What does the API response (resp.prefixes) look like? I don't feel like giving the dev a prefixes property on the files array is too important or useful. If they're using the folder property on a query passed to getFiles, they may not even realize we're using "prefixes" and "delimiters" under the hood, and wouldn't know what to do with that.

@stephenplusplus
Copy link
Contributor

in progress and ready for review are relics from the days of waffle.io. opinions wanted and enhancement (and really, anything except for the service a PR is affecting) make more sense to me to be used on issues. But, I won't remove them again if you would like them back :)

@ryanseys
Copy link
Contributor Author

What does the API response (resp.prefixes) look like?

Looks something like [ 'test/goodbye/', 'test/hello/' ] when folder is set to test so basically, it's the folders in a given folder. What if we used a new query flag that the developer can specify and then we will return them in the prefixes property. Like includePrefixes: true or something?

@stephenplusplus
Copy link
Contributor

What if on the File object itself, it had a "folder" property that was it's prefix? I don't think an array of prefixes is too useful.

@ryanseys
Copy link
Contributor Author

Yeah, thought of that too! A complaint I have with that with doing that is that suddenly everyone's code will need to handle the case where the file isn't actually a file and has no metadata or id or anything, and just contains a folder property instead.

@stephenplusplus
Copy link
Contributor

How would that happen? I'm envisioning:

bucket.getFiles(function(err, files) {
  files[0].name = 'iamin/a/folder.jpg'
  files[0].folder = 'iamin/a'

  files[1].name = 'iamnotinafolder.jpg'
  files[0].folder = undefined
});

Though, chasing this thought down, I'm still not sure that's a helpful bit of knowledge to have. If I really wanted to, I could use the native path module to parse out the folder/extension from a file path. The most important thing should be bucket.getFiles({ folder: 'a/folder' }) to get only the files from a folder, and bucket.getFolders() (which I don't think we know how to do yet / haven't implemented).

@ryanseys
Copy link
Contributor Author

The problem with your example is that the files might not exist but the folders would. Also, bucket.getFiles({ folder: 'test' }) returns the files in the test folder (and the folder itself), and prefixes returns the folders in that folder.

@ryanseys
Copy link
Contributor Author

Because, the test folder is returned the results, we could add the prefixes as a subfolders property on that object in the files list? This won't work for the root folder (/) because it's not a folder, it's just the root. In that case, getting the subfolders in the root would be impossible which isn't ideal.

@stephenplusplus
Copy link
Contributor

My head is spinning a bit, because I still don't see the value to a user. There's clear value to bucket.getFiles({ folder: 'a/folder' }), and bucket.getFolders() would be nice as well, though not as vital. Even for a power-user, getting a File object that retains knowledge of the folder it's in or sub-folder, or prefix, etc, isn't something I can see being useful. And if I'm wrong and the user does need that information, they can solve the problem simply by putting files that were retrieved from a folder into a data structure that's labeled as such:

var folders = { dir1: [], dir2: [] };

Object.keys(folders).forEach(function(folder) {
  bucket.getFiles({ folder: folder }, function (err, files) {
    folders[folder] = files;
  });
});

Hope I'm not overlooking something obvious. As is evident from my history on this project, it sometimes just takes a while for me to understand the Google APIs :)

// also, would it be useful / possible to accept getFiles({ folders: ['folder1', 'folder2'] })?

@ryanseys
Copy link
Contributor Author

Yeah my head is spinning too. In this case I think it's best if we just find the simplest way to return the parameter rather than potentially messing up some folder-related methods that we don't fully understand.

@ryanseys
Copy link
Contributor Author

Ok, no folder value, how about just returning prefixes so that this value can be used to allow developers to create their own similar functionality if required (e.g. like in #342)

@stephenplusplus
Copy link
Contributor

I'll let you make the call on this.

@ryanseys
Copy link
Contributor Author

Yeah, I've decided this idea sucks. Will reconsider #342 or something similar.

@ryanseys ryanseys closed this Jan 27, 2015
@ryanseys ryanseys deleted the storage-folders branch January 27, 2015 16:14
chingor13 pushed a commit that referenced this pull request Aug 22, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@types/mocha](https://togithub.com/DefinitelyTyped/DefinitelyTyped) | devDependencies | major | [`^7.0.2` -> `^8.0.0`](https://renovatebot.com/diffs/npm/@types%2fmocha/7.0.2/8.0.0) |

---

### Renovate configuration

:date: **Schedule**: "after 9am and before 3pm" (UTC).

:vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

:recycle: **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-asset).
sofisl pushed a commit that referenced this pull request Nov 11, 2022
sofisl pushed a commit that referenced this pull request Nov 11, 2022
The library is regenerated with gapic-generator-typescript v1.3.1.

Committer: @alexander-fenster
PiperOrigin-RevId: 372468161

Source-Link: googleapis/googleapis@75880c3

Source-Link: googleapis/googleapis-gen@77b1804
sofisl pushed a commit that referenced this pull request Nov 11, 2022
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [jsdoc-region-tag](https://togithub.com/googleapis/jsdoc-region-tag) | [`^1.0.2` -> `^2.0.0`](https://renovatebot.com/diffs/npm/jsdoc-region-tag/1.3.1/2.0.0) | [![age](https://badges.renovateapi.com/packages/npm/jsdoc-region-tag/2.0.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/jsdoc-region-tag/2.0.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/jsdoc-region-tag/2.0.0/compatibility-slim/1.3.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/jsdoc-region-tag/2.0.0/confidence-slim/1.3.1)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>googleapis/jsdoc-region-tag</summary>

### [`v2.0.0`](https://togithub.com/googleapis/jsdoc-region-tag/blob/HEAD/CHANGELOG.md#&#8203;200-httpsgithubcomgoogleapisjsdoc-region-tagcomparev131v200-2022-05-20)

[Compare Source](https://togithub.com/googleapis/jsdoc-region-tag/compare/v1.3.1...v2.0.0)

##### ⚠ BREAKING CHANGES

-   update library to use Node 12 ([#&#8203;107](https://togithub.com/googleapis/jsdoc-region-tag/issues/107))

##### Build System

-   update library to use Node 12 ([#&#8203;107](https://togithub.com/googleapis/jsdoc-region-tag/issues/107)) ([5b51796](https://togithub.com/googleapis/jsdoc-region-tag/commit/5b51796771984cf8b978990025f14faa03c19923))

##### [1.3.1](https://www.github.com/googleapis/jsdoc-region-tag/compare/v1.3.0...v1.3.1) (2021-08-11)

##### Bug Fixes

-   **build:** migrate to using main branch ([#&#8203;79](https://www.togithub.com/googleapis/jsdoc-region-tag/issues/79)) ([5050615](https://www.github.com/googleapis/jsdoc-region-tag/commit/50506150b7758592df5e389c6a5c3d82b3b20881))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-datalabeling).
sofisl pushed a commit that referenced this pull request Nov 11, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@types/mocha](https://togithub.com/DefinitelyTyped/DefinitelyTyped) | devDependencies | major | [`^7.0.2` -> `^8.0.0`](https://renovatebot.com/diffs/npm/@types%2fmocha/7.0.2/8.0.0) |

---

### Renovate configuration

:date: **Schedule**: "after 9am and before 3pm" (UTC).

:vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

:recycle: **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-asset).
sofisl pushed a commit that referenced this pull request Jan 17, 2023
sofisl pushed a commit that referenced this pull request Jan 24, 2023
sofisl pushed a commit that referenced this pull request Jan 25, 2023
sofisl pushed a commit that referenced this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants