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

Globs #140

Merged
merged 9 commits into from
Mar 2, 2019
Merged

Globs #140

merged 9 commits into from
Mar 2, 2019

Conversation

skoschik
Copy link
Contributor

#122
Hey guys, I like this idea but I had a slightly different take on implementing it... And I'm just gonna put it out there with a PR.
What about passing another option property to getDirectoryContents and letting the filtering happen there? I like getDirectoryContents because we can pull a single directory or pull everything with an infinite depth request. I wasn't sure if getMatchingPaths was intended on doing an infinite depth request or just working in a single directory.

I used the minimatch library from isaacs (it's a dependency of node-glob 👍 )

Thanks for letting me butt in

Copy link
Owner

@perry-mitchell perry-mitchell left a comment

Choose a reason for hiding this comment

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

Some changes..

@@ -13,7 +13,7 @@ function getDirectoryContents(remotePath, options) {
method: "PROPFIND",
headers: {
Accept: "text/plain",
Depth: 1
Depth: options.deep ? "infinity" : 1
Copy link
Owner

Choose a reason for hiding this comment

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

This already exists in master.. perhaps you could merge/rebase your branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully I cleaned up the history correctly. I'm willing to try again if necessary.

@@ -1,7 +1,7 @@
const pathPosix = require("path-posix");
const joinURL = require("url-join");
const { merge } = require("../merge.js");
const { handleResponseCode, processResponsePayload } = require("../response.js");
const { handleResponseCode, processResponsePayload, processGlobFilter } = require("../response.js");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you alphabetise this please?

if (options.glob && options.glob.pattern) {
return processGlobFilter(files, options.glob);
}
return files;
Copy link
Owner

Choose a reason for hiding this comment

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

This could perhaps be made a tad simpler, something like .then(files => options.glob && options.glob.pattern ? processGlobFilter(files, options.glob) : files)

@@ -1,3 +1,5 @@
import minimatch from "minimatch";
Copy link
Owner

Choose a reason for hiding this comment

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

This should be a commonjs require

module.exports = {
handleResponseCode,
processResponsePayload
processResponsePayload,
processGlobFilter
Copy link
Owner

Choose a reason for hiding this comment

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

This could be alphabetised..

@@ -18,7 +20,12 @@ function processResponsePayload(response, data, isDetailed = false) {
: data;
}

function processGlobFilter(files, glob) {
Copy link
Owner

Choose a reason for hiding this comment

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

This could be alphabetised..

@@ -18,7 +18,8 @@ function createServer(dir, authType) {
const server = new ws.WebDAVServer({
port: 9988,
httpAuthentication: auth,
privilegeManager: privilegeManager
privilegeManager: privilegeManager,
maxRequestDepth: Infinity
Copy link
Owner

Choose a reason for hiding this comment

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

This is also already in master. It should be handled by a rebase.

@@ -104,4 +104,22 @@ describe("getDirectoryContents", function() {
.that.matches(/^[a-f0-9]{32}$/);
});
});

it("returns all directory contents when deep = true which translates to depth:'infinity')", function() {
Copy link
Owner

Choose a reason for hiding this comment

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

Already in master..

});
});

it("glob filter test')", function() {
Copy link
Owner

Choose a reason for hiding this comment

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

Extra ) in title. The name could perhaps be better - supports globbing files perhaps.

Copy link
Owner

@perry-mitchell perry-mitchell left a comment

Choose a reason for hiding this comment

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

Looks good!

@perry-mitchell
Copy link
Owner

Thank you @skoschik!

@perry-mitchell perry-mitchell merged commit 966fd52 into perry-mitchell:master Mar 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants