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

Add Core #46

Merged
merged 9 commits into from
Dec 3, 2015
Merged

Add Core #46

merged 9 commits into from
Dec 3, 2015

Conversation

10sr
Copy link
Member

@10sr 10sr commented Nov 25, 2015

What is This?

Pull Request to add Core implemented in Emacs Lisp

Done

  • Add core el files
    • From here
      • editorconfig-core
      • editorconfig-core-handle
    • From here
      • editorconfig-fnmatch
  • Add files and a submodule to use to test core functions
  • Update Makefile for core test
  • Update document
  • Update function to use el core as a fallback
    • As commented 👇, by default editorconfig-apply now first try to invoke editorconfig core executable, and fallback to elisp implementation if it is not found
    • I added new editorconfig-get-properties
      • It is set to be the default value of editorconfig-get-properties-function
      • Existing editorconfig-get-properties was renamed to editorconfig-call-editorconfig-exec: I think it is more suited
  • Bump version
    • v0.6

I'll Do After Merge

  • Deprecate my repositories
  • Update MELPA recipe

@xuhdev
Copy link
Member

xuhdev commented Nov 27, 2015

I just have one comment here: after we add the core, I still suggest to use the external editorconfig command as the default way, and only if the extension cannot find them, we use the elisp implemented core. The reason is simple: we have the EditorConfig cores written in more common languages and we would have more support for them. While the elisp core is good work, we still want to make the world less discrepant if possible.

@10sr
Copy link
Member Author

10sr commented Nov 27, 2015

That's reasonable. I'll do so 🙋

@10sr 10sr force-pushed the addCore branch 2 times, most recently from 286469b to f559275 Compare November 28, 2015 15:42
@10sr
Copy link
Member Author

10sr commented Nov 28, 2015

NOTE: I'm going to wait #47 to be merged because I want to change the function name of editorconfig-get-properties.

@10sr 10sr force-pushed the addCore branch 6 times, most recently from 26ebf11 to 0ec53a2 Compare November 28, 2015 17:18
@10sr 10sr force-pushed the addCore branch 2 times, most recently from b1ae861 to f57995a Compare November 30, 2015 17:13
@10sr
Copy link
Member Author

10sr commented Nov 30, 2015

Now I believe I've almost finished my work.
I will merge this PR in a few days, but please feel free to comment if you have something.

@10sr 10sr changed the title [WIP] Add Core Add Core Nov 30, 2015
10sr added 4 commits December 1, 2015 03:18
editorconfig-core-get-properties-hash will be called when editorconfig
executable is not found.
@xuhdev
Copy link
Member

xuhdev commented Nov 30, 2015

@10sr You don't have to wait for someone else to merge #47---If you think #47 is good enough, you can merge it yourself :)

@10sr
Copy link
Member Author

10sr commented Dec 1, 2015

Right! Thanks 😈

@@ -0,0 +1,102 @@
#!/bin/sh
:;#-*-Emacs-Lisp-*-
Copy link
Member

Choose a reason for hiding this comment

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

Just one comment, can you add the license header to this file as well? This file seems significantly long enough, so without a license header its license would be ambitious---since we have a license in the root directory but no license (proprietary by default) in the header.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I'll add that tonight (about 6 hours later 😺) and also merge this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

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