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

Merging bundle-el into el-get #1976

Merged
merged 27 commits into from Jan 6, 2015

Conversation

Projects
None yet
4 participants
@tarao
Collaborator

tarao commented Nov 1, 2014

As we discussed in #1967, I would like to merge my bundle-el to el-get.

  • bundle.el and eval-after-load-compile.el will be merged as el-get-bundle.el and el-get-eval-after-load-compile.el respectively
  • Related functions are added to el-get.el as autoloads (They are not required since they are not needed by the el-get's core functionality)
  • No documentation is provided: I'm not sure where to add
@npostavs

This comment has been minimized.

Show comment
Hide comment
@npostavs

npostavs Nov 1, 2014

Collaborator

The eval-after-load-compile.el seems overly complicated. Just lambda quoting would suffice. Emacs 24.4 provides this as with-eval-after-load so we could use the same name:

(unless (fboundp 'with-eval-after-load)
  (defmacro with-eval-after-load (file &rest body)
    "Execute BODY after FILE is loaded.
FILE is normally a feature name, but it can also be a file name,
in case that file does not provide any feature."
    (declare (indent 1) (debug t))
    `(eval-after-load ,file (lambda () ,@body)))

Also there is a possible licensing issue: bundle-el appears to be GPL, but el-get is WTFPL. @dimitri has indicated that he doesn't want to license el-get as GPL.

Collaborator

npostavs commented Nov 1, 2014

The eval-after-load-compile.el seems overly complicated. Just lambda quoting would suffice. Emacs 24.4 provides this as with-eval-after-load so we could use the same name:

(unless (fboundp 'with-eval-after-load)
  (defmacro with-eval-after-load (file &rest body)
    "Execute BODY after FILE is loaded.
FILE is normally a feature name, but it can also be a file name,
in case that file does not provide any feature."
    (declare (indent 1) (debug t))
    `(eval-after-load ,file (lambda () ,@body)))

Also there is a possible licensing issue: bundle-el appears to be GPL, but el-get is WTFPL. @dimitri has indicated that he doesn't want to license el-get as GPL.

@tarao

This comment has been minimized.

Show comment
Hide comment
@tarao

tarao Nov 2, 2014

Collaborator

The eval-after-load-compile.el seems overly complicated.

Oh, you are right. And I've noticed that the functionality should be provided as another repository since the problem it tackles is not specific to el-get but is common to any case byte-compiling an eval-after-load form with referring to variables defiened in the loading package. So, I removed it from this pull request and moved it to https://github.com/tarao/with-eval-after-load-feature-el.

Also there is a possible licensing issue

I see. No problem with changing the license of my code. Fixed in bf97962.

Collaborator

tarao commented Nov 2, 2014

The eval-after-load-compile.el seems overly complicated.

Oh, you are right. And I've noticed that the functionality should be provided as another repository since the problem it tackles is not specific to el-get but is common to any case byte-compiling an eval-after-load form with referring to variables defiened in the loading package. So, I removed it from this pull request and moved it to https://github.com/tarao/with-eval-after-load-feature-el.

Also there is a possible licensing issue

I see. No problem with changing the license of my code. Fixed in bf97962.

@dimitri

This comment has been minimized.

Show comment
Hide comment
@dimitri

dimitri Nov 2, 2014

Owner

Thanks for your pull request, and for accepting to re-license over to WTFPL!

Merging your code will only be possible with proper updates to the documentation of course. You need to be editing the el-get.texi file then just type make el-get.info to get the distributed file. We commit both to the repository to ease bootstrapping el-get: we want users to see docs easily.

Also I think that the main README.md file should be edited to provide for some intro to el-get-bundle as an alternative way to setup el-get.

On the source parts, I see that @npostavs took the lead in reviewing and that's very good news ;-)

Owner

dimitri commented Nov 2, 2014

Thanks for your pull request, and for accepting to re-license over to WTFPL!

Merging your code will only be possible with proper updates to the documentation of course. You need to be editing the el-get.texi file then just type make el-get.info to get the distributed file. We commit both to the repository to ease bootstrapping el-get: we want users to see docs easily.

Also I think that the main README.md file should be edited to provide for some intro to el-get-bundle as an alternative way to setup el-get.

On the source parts, I see that @npostavs took the lead in reviewing and that's very good news ;-)

Show outdated Hide outdated el-get-bundle.el
Show outdated Hide outdated el-get-bundle.el
Show outdated Hide outdated el-get-bundle.el
Show outdated Hide outdated el-get-bundle.el
Show outdated Hide outdated el-get-bundle.el
Show outdated Hide outdated el-get-bundle.el
Show outdated Hide outdated el-get-bundle.el
@npostavs

This comment has been minimized.

Show comment
Hide comment
@npostavs

npostavs Nov 15, 2014

Collaborator

All the new changes look good.

We should look at the documentation. I think bundle-el's original README is too detailed to put in el-get's README, so that info should go in the manual. But also, I feel it's missing some details, because I don't quite understand what bundle-el is doing beside the syntactical sugar. There's some code for registering "call-sites" which I haven't figured out.

There is an el-get-bundle-update function. If I use el-get-update instead, will the el-get-bundle code get confused?

;; (el-get-bundle FEATURE in PACKAGE ...) form

This syntax isn't documented in the README.

Collaborator

npostavs commented Nov 15, 2014

All the new changes look good.

We should look at the documentation. I think bundle-el's original README is too detailed to put in el-get's README, so that info should go in the manual. But also, I feel it's missing some details, because I don't quite understand what bundle-el is doing beside the syntactical sugar. There's some code for registering "call-sites" which I haven't figured out.

There is an el-get-bundle-update function. If I use el-get-update instead, will the el-get-bundle code get confused?

;; (el-get-bundle FEATURE in PACKAGE ...) form

This syntax isn't documented in the README.

@dimitri

This comment has been minimized.

Show comment
Hide comment
@dimitri

dimitri Nov 16, 2014

Owner

Thanks for making progress on this! Still not seeing the manual bits in the pull request though. Is more help needed to edit the manual parts?

Owner

dimitri commented Nov 16, 2014

Thanks for making progress on this! Still not seeing the manual bits in the pull request though. Is more help needed to edit the manual parts?

@npostavs

This comment has been minimized.

Show comment
Hide comment
@npostavs

npostavs Dec 7, 2014

Collaborator

@tarao: ping?

Collaborator

npostavs commented Dec 7, 2014

@tarao: ping?

@tarao

This comment has been minimized.

Show comment
Hide comment
@tarao

tarao Dec 8, 2014

Collaborator

@npostavs @dimitri

There's some code for registering "call-sites" which I haven't figured out.

That is for maintaining a byte-compiled piece of initialization code. Each initialization code is stored in el-get-bundle-init-dir in order to store the byte-compiled version of it. "call-sites" are now used only for deciding a file name of the piece of code. (Registering correspondence between a call-site and a package has been removed in 59b7874.)

There is an el-get-bundle-update function. If I use el-get-update instead, will the el-get-bundle code get confused?

Yes, it was, but I could fix it not to get confused. The reason for providing el-get-bundle-update was to reload an user init file after el-get-update. The reason for reloading in an update sequence was that el-get-bundle-init-count-alist needed to be reset on reloading the user init file, where el-get-bundle-init-count-alist maintains (a part of) the file name of initialization code. I fixed it to reset el-get-bundle-init-count-alist at after-load-functions in a50b80d. So, el-get-bundle-update is not needed any more (removed in 11695ef 07ff98a 90f2366).

This syntax isn't documented in the README.
Still not seeing the manual bits in the pull request though.

I've written sections in README.md and el-get.texi ( 45a8626 b43b226 4b42890 ).

Collaborator

tarao commented Dec 8, 2014

@npostavs @dimitri

There's some code for registering "call-sites" which I haven't figured out.

That is for maintaining a byte-compiled piece of initialization code. Each initialization code is stored in el-get-bundle-init-dir in order to store the byte-compiled version of it. "call-sites" are now used only for deciding a file name of the piece of code. (Registering correspondence between a call-site and a package has been removed in 59b7874.)

There is an el-get-bundle-update function. If I use el-get-update instead, will the el-get-bundle code get confused?

Yes, it was, but I could fix it not to get confused. The reason for providing el-get-bundle-update was to reload an user init file after el-get-update. The reason for reloading in an update sequence was that el-get-bundle-init-count-alist needed to be reset on reloading the user init file, where el-get-bundle-init-count-alist maintains (a part of) the file name of initialization code. I fixed it to reset el-get-bundle-init-count-alist at after-load-functions in a50b80d. So, el-get-bundle-update is not needed any more (removed in 11695ef 07ff98a 90f2366).

This syntax isn't documented in the README.
Still not seeing the manual bits in the pull request though.

I've written sections in README.md and el-get.texi ( 45a8626 b43b226 4b42890 ).

@dimitri

This comment has been minimized.

Show comment
Hide comment
@dimitri

dimitri Dec 8, 2014

Owner

Thanks for the update, I think we're soon to be able to merge!

I just reviewed the docs part, and I think the texi parts need more example around the advanced options of how to specify a bundle (gist, types, etc). Please add some example and turn the list into subsections.

Owner

dimitri commented Dec 8, 2014

Thanks for the update, I think we're soon to be able to merge!

I just reviewed the docs part, and I think the texi parts need more example around the advanced options of how to specify a bundle (gist, types, etc). Please add some example and turn the list into subsections.

@priyadarshan

This comment has been minimized.

Show comment
Hide comment
@priyadarshan

priyadarshan Dec 8, 2014

So much work to do for a good merge. Thank you so much for all this. I am sure I am not the only one that can't wait to see el-bundle splendidly merged into el-get core. Thank you so much!

priyadarshan commented Dec 8, 2014

So much work to do for a good merge. Thank you so much for all this. I am sure I am not the only one that can't wait to see el-bundle splendidly merged into el-get core. Thank you so much!

@tarao

This comment has been minimized.

Show comment
Hide comment
@tarao

tarao Dec 13, 2014

Collaborator

@dimitri

I added examples from the original README and fixed some parts to fit in the info.

Collaborator

tarao commented Dec 13, 2014

@dimitri

I added examples from the original README and fixed some parts to fit in the info.

@dimitri

This comment has been minimized.

Show comment
Hide comment
@dimitri

dimitri Dec 13, 2014

Owner

Thanks @tarao, I'm now pretty happy with the docs! @npostavs : do you still have any concerns with the current version of the code in the pull request? Please consider merging if you're fine with it!

@tarao: would you be interested into joining the el-get maintenance team so that you can directly fix any bug reported on el-get-bundle as soon as the feature is in? In which case, you might also want to do the merge yourself (once we get the "clear" from @npostavs of course)...

Owner

dimitri commented Dec 13, 2014

Thanks @tarao, I'm now pretty happy with the docs! @npostavs : do you still have any concerns with the current version of the code in the pull request? Please consider merging if you're fine with it!

@tarao: would you be interested into joining the el-get maintenance team so that you can directly fix any bug reported on el-get-bundle as soon as the feature is in? In which case, you might also want to do the merge yourself (once we get the "clear" from @npostavs of course)...

@npostavs

This comment has been minimized.

Show comment
Hide comment
@npostavs

npostavs Dec 14, 2014

Collaborator

One small nit pick about the code: (or (and (listp X) X) (list X)) in a few places should use el-get-as-list instead.

A bit more big picture, I'm wondering if the init compilation stuff can be more integrated more with the existing el-get init-<package-name>.el init files. Somehow it doesn't need this "callsite" concept and the hooks for recompilation. I'm not sure if it's because (a) the current el-get-bundle code is currently over-complicated, (b) the current el-get code doesn't cover some edge cases, or (c) having the init files be generated from forms means there are more edge cases to cover. I think we can trust @tarao to make the call about that.

My only real concern before merging is that The el-get-bundle macro info section is a copy of the el-get-bundle docstring. This will lead to the same problem as the el-get-sources variable in which the manual and docstring copies have diverged, so I think we really shouldn't repeat that mistake.

Collaborator

npostavs commented Dec 14, 2014

One small nit pick about the code: (or (and (listp X) X) (list X)) in a few places should use el-get-as-list instead.

A bit more big picture, I'm wondering if the init compilation stuff can be more integrated more with the existing el-get init-<package-name>.el init files. Somehow it doesn't need this "callsite" concept and the hooks for recompilation. I'm not sure if it's because (a) the current el-get-bundle code is currently over-complicated, (b) the current el-get code doesn't cover some edge cases, or (c) having the init files be generated from forms means there are more edge cases to cover. I think we can trust @tarao to make the call about that.

My only real concern before merging is that The el-get-bundle macro info section is a copy of the el-get-bundle docstring. This will lead to the same problem as the el-get-sources variable in which the manual and docstring copies have diverged, so I think we really shouldn't repeat that mistake.

@tarao

This comment has been minimized.

Show comment
Hide comment
@tarao

tarao Dec 20, 2014

Collaborator

@dimitri

would you be interested into joining the el-get maintenance team so that you can directly fix any bug reported on el-get-bundle as soon as the feature is in?

Thanks for the offer! I'm grad to be a maintainer.

Collaborator

tarao commented Dec 20, 2014

@dimitri

would you be interested into joining the el-get maintenance team so that you can directly fix any bug reported on el-get-bundle as soon as the feature is in?

Thanks for the offer! I'm grad to be a maintainer.

@tarao

This comment has been minimized.

Show comment
Hide comment
@tarao

tarao Dec 20, 2014

Collaborator

@npostavs

One small nit pick about the code: (or (and (listp X) X) (list X)) in a few places should use el-get-as-list instead.

Fixed in a6ac46c.

I'm wondering if the init compilation stuff can be more integrated more with the existing el-get init-<package-name>.el init files. Somehow it doesn't need this "callsite" concept and the hooks for recompilation.

I think it's because (c) having the init files be generated from forms means there are more edge cases to cover. Each init file corresponds to a form but neither to a package nor to an user init file (.emacs). There can be many forms for a single package when you write multiple (el-get-bundle PACKAGE ...) and these may not be in the same file (when they are located in multiple files loaded from .emacs). When the form has been changed, el-get-bundle must indentify which init file should be recompiled by the occurence of el-get-bundle macro call, neither by the package name nor by the file in which the macro call is located. That is the concept of "callsite".

This will lead to the same problem as the el-get-sources variable in which the manual and docstring copies have diverged

Oh, is there any good way to avoid this?

Collaborator

tarao commented Dec 20, 2014

@npostavs

One small nit pick about the code: (or (and (listp X) X) (list X)) in a few places should use el-get-as-list instead.

Fixed in a6ac46c.

I'm wondering if the init compilation stuff can be more integrated more with the existing el-get init-<package-name>.el init files. Somehow it doesn't need this "callsite" concept and the hooks for recompilation.

I think it's because (c) having the init files be generated from forms means there are more edge cases to cover. Each init file corresponds to a form but neither to a package nor to an user init file (.emacs). There can be many forms for a single package when you write multiple (el-get-bundle PACKAGE ...) and these may not be in the same file (when they are located in multiple files loaded from .emacs). When the form has been changed, el-get-bundle must indentify which init file should be recompiled by the occurence of el-get-bundle macro call, neither by the package name nor by the file in which the macro call is located. That is the concept of "callsite".

This will lead to the same problem as the el-get-sources variable in which the manual and docstring copies have diverged

Oh, is there any good way to avoid this?

@dimitri

This comment has been minimized.

Show comment
Hide comment
@dimitri

dimitri Dec 20, 2014

Owner

Welcome @tarao as a new collaborator (github speak) to the project! You're allowed to push now, please consider helping with recipe maintenance and merging your own bundle branch as soon as the review is all clear, which I feel is going to happen soon, and in any case is in the hands of @npostavs now!

Thanks again guys for a very good work!

Owner

dimitri commented Dec 20, 2014

Welcome @tarao as a new collaborator (github speak) to the project! You're allowed to push now, please consider helping with recipe maintenance and merging your own bundle branch as soon as the review is all clear, which I feel is going to happen soon, and in any case is in the hands of @npostavs now!

Thanks again guys for a very good work!

@npostavs

This comment has been minimized.

Show comment
Hide comment
@npostavs

npostavs Jan 5, 2015

Collaborator

My only real concern before merging is that The el-get-bundle macro info section is a copy of the el-get-bundle docstring. This will lead to the same problem as the el-get-sources variable in which the manual and docstring copies have diverged, so I think we really shouldn't repeat that mistake.

Um sorry, I think this was actually already fixed by b4ee555. I must have been looking at an old revision. Just speaking generally, the manual should have more details and example usage, while docstrings should have just the minimum summary of the behaviour.

There can be many forms for a single package when you write multiple (el-get-bundle PACKAGE ...) and these may not be in the same file (when they are located in multiple files loaded from .emacs).

So multiple calls to el-get-bundle get merged? Knowing this, your code makes a lot more sense now. :) Perhaps you can add a note about that to the manual.

Okay, I think that's it for me. @tarao: feel free to (finally) install this. :)

Collaborator

npostavs commented Jan 5, 2015

My only real concern before merging is that The el-get-bundle macro info section is a copy of the el-get-bundle docstring. This will lead to the same problem as the el-get-sources variable in which the manual and docstring copies have diverged, so I think we really shouldn't repeat that mistake.

Um sorry, I think this was actually already fixed by b4ee555. I must have been looking at an old revision. Just speaking generally, the manual should have more details and example usage, while docstrings should have just the minimum summary of the behaviour.

There can be many forms for a single package when you write multiple (el-get-bundle PACKAGE ...) and these may not be in the same file (when they are located in multiple files loaded from .emacs).

So multiple calls to el-get-bundle get merged? Knowing this, your code makes a lot more sense now. :) Perhaps you can add a note about that to the manual.

Okay, I think that's it for me. @tarao: feel free to (finally) install this. :)

@tarao

This comment has been minimized.

Show comment
Hide comment
@tarao

tarao Jan 6, 2015

Collaborator

the manual should have more details and example usage, while docstrings should have just the minimum summary of the behaviour

I get your point now.

Perhaps you can add a note about that to the manual.

Added in 05605bd.

Now I merge this pull request. Thank you @npostavs, @dimitri for the review, and @priyadarshan, who have suggested merging bundle-el to el-get.

Collaborator

tarao commented Jan 6, 2015

the manual should have more details and example usage, while docstrings should have just the minimum summary of the behaviour

I get your point now.

Perhaps you can add a note about that to the manual.

Added in 05605bd.

Now I merge this pull request. Thank you @npostavs, @dimitri for the review, and @priyadarshan, who have suggested merging bundle-el to el-get.

tarao added a commit that referenced this pull request Jan 6, 2015

Merge pull request #1976 from tarao/bundle
Merging bundle-el into el-get

@tarao tarao merged commit 175a5f3 into dimitri:master Jan 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@dimitri

This comment has been minimized.

Show comment
Hide comment
@dimitri

dimitri Jan 6, 2015

Owner

Awesome, thanks!

Owner

dimitri commented Jan 6, 2015

Awesome, thanks!

@dimitri

This comment has been minimized.

Show comment
Hide comment
@priyadarshan

This comment has been minimized.

Show comment
Hide comment
@priyadarshan

priyadarshan Jan 6, 2015

Thank you so much!

A very happy el-get user

priyadarshan commented Jan 6, 2015

Thank you so much!

A very happy el-get user

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment