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

"/**/*.dart" should match any absolute Dart file path on any OS #2

Closed
scheglov opened this issue Nov 18, 2015 · 8 comments
Closed

"/**/*.dart" should match any absolute Dart file path on any OS #2

scheglov opened this issue Nov 18, 2015 · 8 comments
Assignees
Labels
type-enhancement A request for a change that isn't a bug

Comments

@scheglov
Copy link

@nex3 @munificent @bwilkerson

Asking users to think about differences between Posix and Windows styles is too much.
Especially when path separators are already / for any OS.

I don't also quite understand the requirement for the leading / to match absolute paths.
It seems confusing.

@scheglov
Copy link
Author

FYI - we cannot switch for using package:glob until this is fixed.

We are going to send these patterns back to IDEs, and we don't want them to understand idiosyncrasies of absolute paths matching.

Ideally it should be even just **.dart without all the rites to distinguish absolute and relative paths.
We don't list files using package:glob, so I think we don't care about differences between absolute and relative globs.

@nex3
Copy link
Member

nex3 commented Nov 18, 2015

I don't think we can exactly interpret / as a shorthand for "any absolute path". The rule for glob matching is that a glob that will be listed using Glob.list if and only if it's matched using Glob.match, but that falls down if we add this shorthand—we can't list every network drive on Windows even if we wanted to.

That said, a leading slash does have a meaning on Windows that we could use here. On Windows, a leading slash denotes a root-relative path; that is, \foo means X:\foo where X is the drive of the working directory. Currently we don't really support root-relative globs, but doing so would better match the rule. Would it work for you if /foo matched all absolute paths on the current drive?

I don't also quite understand the requirement for the leading / to match absolute paths.
It seems confusing.

This comes from the same rule. We want a glob to mean the same thing regardless of how you use it. If a user writes new Glob("**.dart").list(), they obviously want to list all Dart files in the current directory, not all Dart files on their machine. To be consistent, the same glob matches paths to Dart files in the current directory.

If it would help, we could add an allowAbsolute flag to the constructor that would allow a glob to match absolute paths (although of course this would make listing not work). You can also do this yourself by prepending {/,*:/} to the glob.

We are going to send these patterns back to IDEs, and we don't want them to understand idiosyncrasies of absolute paths matching.

I'm not sure what you mean by "send these patterns back"—are you expecting IDEs to implement their own globbing logic? Or are you trying to match existing IDE behavior?

You don't have to send IDEs the literal glob that's used by the glob package. Just like creating a recursive glob implicitly adds {,**} to the end of the pattern, it's fine to have the strings that users write not exactly match the strings that the glob package consumes.

@scheglov
Copy link
Author

The IDE need to know which files DAS analyses to know whether it should send changes as user types to get as you type errors. So, we need just the simplest thing which can be applied to absolute paths. And BTW, current directory can be different for DAS and IDE.

Could we have a simpleMode flag which will support only ?, * and **?
With ** matching any character, even root and drive letters?

@nex3
Copy link
Member

nex3 commented Nov 24, 2015

And BTW, current directory can be different for DAS and IDE.

I don't understand this. What directory you consider a path or a glob relative to is something you can explicitly define. Why not just define these globs as relative to the package root?

If you don't apply the globs to relative paths, how can users express things like "don't analyze anything in my test directory"?

Could we have a simpleMode flag which will support only ?, * and **?
With ** matching any character, even root and drive letters?

We definitely can't have a flag that fundamentally changes the semantics of **. We could add a flag that rejected certain constructs, but I don't really see a reason to centralize that logic—what if another user wanted to reject ** but allow {}? It also seems really easy to check for disallowed characters in user code.

@matanlurey
Copy link
Contributor

Can we close this if it won't be acted on and perhaps is no longer needed?

@nex3
Copy link
Member

nex3 commented Jun 8, 2017

It's still probably a good idea for /** to match all files on the current drive in Windows.

@matanlurey
Copy link
Contributor

Should we rename the issue?

@nex3
Copy link
Member

nex3 commented Jun 8, 2017

It might be simpler just to file a new issue, actually. I'll do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants