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

Avoid overly-broad tasklib.find call #3249

Merged
merged 1 commit into from
Dec 13, 2016

Conversation

jmyersmsft
Copy link
Member

No description provided.

@bryanmacfarlane
Copy link
Contributor

bryanmacfarlane commented Dec 13, 2016

want to have @ericsciple take a look?

// include the wildcard character because:
// dirname(c:\foo\bar\) => c:\foo (which will make find() return a bunch of stuff we know we'll discard)
// dirname(c:\foo\bar\*) => c:\foo\bar
let findPathRoot = path.dirname(pattern.slice(0, idx + 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi a find match function was added in newer version of the lib. the firstWildcardIndex isn't entirely accurate.

Copy link
Contributor

Choose a reason for hiding this comment

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

eventually we want to get to a consistent experience across in the box tasks: https://github.com/Microsoft/vsts-task-lib/blob/master/node/docs/findingfiles.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Won't fix. I strongly agree that we should converge on a single file finding experience, but changing matchers is a breaking change, and I'm not interested in changing the observable behavior in this PR.

The scope of this PR is simply to stop enumerating the parent of the directory we're matching against with our existing matcher.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes i agree it is something outside the scope of this pr

the change here seems appropriate

note, even with most of the glob options turned off, it's character sets that are not accounted for by firstWildcardIndex and cannot be turned off

Copy link
Member Author

@jmyersmsft jmyersmsft Dec 13, 2016

Choose a reason for hiding this comment

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

If you want consistency, your team should be opinionated and remove almost all of the options from that interface. Consistent UI will just be even more confusing if each task selects different options underneath.

There should also be structures in the task definition to make writing the UX consistently be easier than not doing so.

We haven't gotten complaints about character sets not working right, and I don't think we've documented anything but *, ** and ?, so I think we'll just leave that to reaction if someone complains

Copy link
Contributor

Choose a reason for hiding this comment

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

for globbing consistency, the strategy is: 1) task author guidance (doc linked above). 2) good defaults for the glob options. changing the defaults for the glob functions was a breaking change in the 2x task lib. 3) update in the box tasks. they are open source examples.

for the UX structures, what do you have in mind? this is straying far from the PR :) you might want to shoot a mail to bryan or chris

@kkdawkins
Copy link
Contributor

kkdawkins commented Dec 13, 2016

:shipit:

@jmyersmsft jmyersmsft merged commit e63952a into microsoft:master Dec 13, 2016
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.

5 participants