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

properties -> pairs #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jednano
Copy link
Member

@jednano jednano commented Dec 3, 2019

As per the specification, we are no longer referring to key-value pairs as properties, but just pairs or key-value pairs.

Copy link
Member

@cxw42 cxw42 left a comment

Choose a reason for hiding this comment

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

In CMakeLists.txt, set_tests_properties, that has to stay PROPERTIES, not be changed to PAIRS. Do the tests pass with these changes? If so, something very strange is happening in cmake land.

@jednano
Copy link
Member Author

jednano commented Dec 3, 2019

That's just the thing. I don't think these tests are being tested without an implementation. How do we go about doing that?

@cxw42
Copy link
Member

cxw42 commented Dec 3, 2019

I usually test with editorconfig-c and editorconfig-vim. After initializing the submodules, I make sure the old tests pass :) . I then cd into the core-test submodule and checkout the PR. Then I run the tests and see what I get.

I could add a Travis/Appveyor setup to run against the vimscript core without too much trouble. I might also be able to do the C core, but that is more challenging on Appveyor because of the Windows build infrastructure.

@jednano
Copy link
Member Author

jednano commented Dec 3, 2019

We should use GitHub Actions. Leave this PR open. I'll figure out the CI.

@cxw42
Copy link
Member

cxw42 commented Dec 4, 2019

@jedmao Sounds good. Do let me know if you need any help with the cmake files or Win-vs.-Unix issues --- I've spent way too much time on both over the last year :) .

I opened #36 so the need for CI would be more visible.

@cxw42 cxw42 mentioned this pull request Dec 4, 2019
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.

2 participants