-
Notifications
You must be signed in to change notification settings - Fork 22
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
132 Add test match with spaces in braces #47
base: master
Are you sure you want to change the base?
Conversation
I wasn't able to run it localy as i'm using windows and cmake doesn't want to build the tests |
you've added a fixture, the tests are there: https://github.com/editorconfig/editorconfig-core-test/blob/master/glob/CMakeLists.txt#L120= |
@greut i added a test for braces with spaces. But still i wasn't able to run the test i get:
|
glob/CMakeLists.txt
Outdated
new_ec_test(braces_word_choice5 braces.in test.py "^choice=true[ \t\n\r]*$") | ||
new_ec_test(braces_word_choice6 braces.in test.js "^choice=true[ \t\n\r]*$") | ||
new_ec_test(braces_word_choice7 braces.in test.html "^choice=true[ \t\n\r]*$") | ||
new_ec_test(braces_word_choice8 braces.in test.pyc "^[ \t\n\r]*$") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line 132, which duplicates line 126.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, how can i test that a extension is not matched based on choice=spaces
might be nice to add a test like that what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Sorry for misunderstanding. Yes, we can test for non-matching. However, the test filename has to be one that is not matched anywhere in the .in
file. How about?---
-new_ec_test(braces_word_choice8 braces.in test.pyc "^[ \t\n\r]*$")
+new_ec_test(braces_word_choice8 braces.in test.NOMATCH "^[ \t\n\r]*$")
Edit why "has to be one that is not matched" --- the new_ec_test()
function runs the core with the given inputs filename and returns all matching key/value pairs. So, if the .in
file has multiple sections, you can't test for whether only one section matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah oke make sense, I thought it might be good idea to test a case that didn't match a specific selector as I had a case that didn't work for me see: editorconfig-checker/editorconfig-checker#198
If you think current test are enough I can agree :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ as of 2e91280
Are we sure this is desirable though? The specification doesn't discuss the case in which spaces are present: https://editorconfig-specification.readthedocs.io/#glob-expressions |
@xuhdev I think the more strict it is the better. So I would say no spaces allowed. But as mentioned here editorconfig-checker/editorconfig-checker#198 (comment) the docs says spaces are allowed. So if we say no spaces I would write a test that a spaces match doesn't match anything. And we should update the docs. |
My interpretation of the spec (and based on super early discussion on the glob format) is that space should not be allowed, because we want to avoid unnecessary workload from implementing glob parsers. Actually, I believe the fact that the absence of a particular pattern should generally imply that it's unavailable rather than that variants are allowed. |
Fixes 132