editorconfig / editorconfig-emacs

EditorConfig plugin for Emacs
https://editorconfig.org
GNU General Public License v3.0
732 stars 103 forks source link

Failure to apply EditorConfig settings in dirs with funny names #345

Closed monnier closed 2 days ago

monnier commented 1 month ago

If I have a ~/[hello]/.editorconfig file with a pattern for /*.c, its settings aren't applied because the code ends up matching ~/[hello]/myfile.c against the pattern ~/[hello]/*.c. We could fix the problem by quoting the special chars like [ but it's kind of silly to do that (and it's more work for fnmatchp and won't hit as often in its cache) when we can instead match myfile.c against *.c.

I pushed a patch which does the latter to the scratch/editorconfig (in nongnu.git). To consult that branch you can do:

git remote add -ft scratch/editorconfig nongnu git://git.sv.gnu.org/emacs/nongnu.git
git log nongnu/scratch/editorconfig

The new code passes all the tests for me. The branch includes a few other changes I think we should include, but do let me know if there's some objection to any of those changes, of course.

monnier commented 2 weeks ago

Ping?

10sr commented 1 week ago

Thanks! 🙌 I fetched it and created a PR for this #346, but the CI fails so I'm checking it...

monnier commented 1 week ago

I fetched it and created a PR for this #346, but the CI fails so I'm checking it...

Hmm... it seems they fail in the test-core part of the tests (judging from the fact that the Windows tests succeed). Not completely surprising since I have not run those tests. I can't see any useful info in the "logs" of the Github tests (all it says is Error:, which is incidentally the exact same content as for the successful(!) tests under Windows).

So I guess I'm going to have to install the rest of the test suite and the tools it uses.

monnier commented 1 week ago

OK, I think I'm beginning to understand how this part of the test suite works. Fixes are on the way.

monnier commented 1 week ago

I fetched it and created a PR for this #346, but the CI fails so I'm checking it...

Hmm... I forced pushed to the same scratch/editorconfig branch code which fixes the regressions I see, but there are still 4 failing tests:

    155 - semicolon_or_hash_in_property (Failed)
    156 - backslashed_semicolon_or_hash_in_property (Failed)
    164 - min_supported_key_length (Failed)
    165 - min_supported_value_length (Failed)

These also fail for me without my patches, so IIUC what you meant by "I'm checking it" is that you're looking in to those failures rather than into the ones I had introduced.

10sr commented 1 week ago

Thanks, really! I pushed again, and it seems to work 🙌 (somehow two cases in the CI hang, but I think it is not related to your patch...)

These also fail for me without my patches, so IIUC what you meant by "I'm checking it" is that you're looking in to those failures rather than into the ones I had introduced.

Hmm, It is not true.. They have never failed for normal cases (both on github actions and my local machine), and they do not on #346 CI, too (except for the two cases). I think the faillures are related to your local environment...

I'll merge this anyway after solving the ci issue.

monnier commented 1 day ago

Closed #345 as completed via #346.

Thanks. BTW, could you give me write access so I can directly push to branches?

xuhdev commented 1 day ago

@monnier Done

monnier commented 1 day ago

@monnier Done

Thanks!