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

Clarify the behavior of leading slash in section title #19

Open
xuhdev opened this issue Apr 17, 2020 · 2 comments
Open

Clarify the behavior of leading slash in section title #19

xuhdev opened this issue Apr 17, 2020 · 2 comments

Comments

@xuhdev
Copy link
Member

xuhdev commented Apr 17, 2020

See the confusion in editorconfig/editorconfig-core-test#37 (comment)

@florianb
Copy link
Member

I'd stick with the .gitignore standard ans make the reference explicit:

The slash / is used as the directory separator. Separators may occur at the beginning, middle or end of the .gitignore search pattern.
If there is a separator at the beginning or middle (or both) of the pattern, then the pattern is relative to the directory level of the particular .gitignore file itself. Otherwise the pattern may also match at any level below the .gitignore level.

@generalmimon
Copy link

@xuhdev:

See the confusion in editorconfig/editorconfig-core-test#37 (comment)

I fully share this "confusion" and agree with @florianb that it would be best to match the behavior of .gitignore (which I'm personally familiar with and intuitively assumed EditorConfig would follow, which turned out not to be the case and I'm not sure why). After all, @dail8859 also assumed the behavior of .gitignore files in editorconfig/editorconfig-core-test#37 (comment) and they were confused why EditorConfig doesn't work like this:

I'm still a bit confused...could you describe the difference between these two in simple English? I will take a guess to give you an idea what I am looking for...these are apparently wrong so any clarification is appreciated.

  • path/separator - matches any file called separator that is any folder called path
  • /top/of/path - matches the exact path /top/of/path under the current working directory

(... just not "current working directory", but the location of .gitignore/.editorconfig, but that's a detail.)

But unlike for @dail8859, the @xuhdev's comment editorconfig/editorconfig-core-test#37 (comment) that was supposed to answer "So does a pattern starting with / have any special meaning?" doesn't clarify the difference for me:

So does a pattern starting with / have any special meaning?

It means the same path as the .editorconfig, because we said on editorconfig.org that it's meant to be the same as gitignore. I believed that we probably have probably missed it in the spec.

Well, right now both path/separator and /top/of/path are interpreted as exact relative paths from the .editorconfig location (i.e. a path/separator pattern is interpreted as /path/separator would be in a .gitignore), so that to me is @xuhdev implying that a pattern starting with / does not have a special meaning. This behavior that sections like [foo/bar.txt] and [/foo/bar.txt] are equivalent is also currently the behavior asserted in tests as correct, as @dail8859 pointed out in editorconfig/editorconfig-core-test#37 (comment):

editorconfig-core-test / filetree/path_separator.in:5-9

[path/separator]
key1=value1

[/top/of/path]
key2=value2
# Tests path separator match
new_ec_test(path_separator path_separator.in path/separator "^key1=value1[ \t\n\r]*$")

# Tests path separator match below top of path
new_ec_test(nested_path_separator path_separator.in nested/path/separator "^[ \t\n\r]*$")
# Tests path separator match top of path only
new_ec_test(top_level_path_separator path_separator.in top/of/path "^key2=value2[ \t\n\r]*$")

# Tests path separator match top of path only
new_ec_test(top_level_path_separator_neg path_separator.in not/top/of/path "^[ \t\n\r]*$")

When you ignore the misleading comments, you see that the corresponding pairs of tests assert exactly the same behavior:

new_ec_test(path_separator           path_separator.in path/separator "^key1=value1[ \t\n\r]*$")
new_ec_test(top_level_path_separator path_separator.in top/of/path    "^key2=value2[ \t\n\r]*$")

new_ec_test(nested_path_separator        path_separator.in nested/path/separator "^[ \t\n\r]*$")
new_ec_test(top_level_path_separator_neg path_separator.in not/top/of/path       "^[ \t\n\r]*$")

As both @dail8859 and @florianb suggested, I also think that it makes more sense to follow the behavior of .gitignore, i.e a section with a pattern not starting with a slash like [path/separator] would match a nested/path/separator path too:

 # Tests path separator match below top of path
-new_ec_test(nested_path_separator path_separator.in nested/path/separator "^[ \t\n\r]*$")
+new_ec_test(nested_path_separator path_separator.in nested/path/separator "^key1=value1[ \t\n\r]*$")

Not only do I think that more people will be familiar with how .gitignore works (because Git is used by almost everyone on a daily basis, but much less people know EditorConfig in the first place and they're even less likely to know about its specifics that are not even documented) so when EditorConfig behaves the same, the user won't run into any unpleasant surprises, I think having a path/separator pattern match recursively is a better default anyway.

First of all, it feels more consistent with "bare" file name patterns without slashes - a [Makefile] pattern also applies not only to /Makefile in the same directory where /.editorconfig is, but to all Makefiles recursively down the directory tree (e.g. /foo/Makefile, /bar/baz/qux/Makefile, etc.). I don't see why a mere change from [Makefile] to [build/Makefile] (i.e. adding of a slash) should break this recursiveness.

Furthermore, when you consider that one use case of .editorconfig would be to lint project files based on the rules written in it (using tools like https://github.com/editorconfig-checker/editorconfig-checker or https://gitlab.com/greut/eclint), interpreting a potentially "ambiguous" (as in we don't know whether the user actually intended the pattern to be recursive or not) pattern like [lib/**.js] as liberally as possible (i.e. recursively) is going to be positive at times and have least negative impact.

The worst thing that can happen when you interpret [lib/**.js] recursively is when the user didn't mean this pattern to be recursive AND the .editorconfig is used in a project where this discrepancy matters (I think this would be very rare in practice). But in that case, the linter could just report problems in more files than expected - the user can either just ignore these or eventually learn about the non-recursive syntax with the leading slash (which would be a valuable knowledge applicable to .gitignore files, too). This is arguably much better than the user assuming that a pattern without the leading slash is applied recursively (as I did, coming from .gitignore syntax) when it in fact is not - in that case, the linter won't ever report any problems for nested files, which the user would normally interpret as a sign that they are flawless (but in fact they aren't checked for some things they were supposed to be checked, which the user wouldn't notice unless they specifically tested for it).

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

No branches or pull requests

3 participants