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 eshell-toggle recipe #6170

Merged
merged 1 commit into from
Jun 4, 2019
Merged

Add eshell-toggle recipe #6170

merged 1 commit into from
Jun 4, 2019

Conversation

a13
Copy link
Contributor

@a13 a13 commented May 18, 2019

Brief summary of what the package does

[Eshell-toggle is another drop-down shell (but can be customized to be a scratch buffer of any kind).]

Direct link to the package repository

https://github.com/4DA/eshell-toggle

Your association with the package

A contributor

Relevant communications with the upstream package maintainer

None needed

Checklist

Please confirm with x:

  • The package is released under a GPL-Compatible Free Software License.
  • I've read CONTRIBUTING.org
  • I've used the latest version of package-lint to check for packaging issues, and addressed its feedback
  • My elisp byte-compiles cleanly
  • M-x checkdoc is happy with my docstrings
  • I've built and installed the package using the instructions in CONTRIBUTING.org
  • I have confirmed some of these without doing them

@riscy
Copy link
Member

riscy commented May 21, 2019

Adding label on compiler error fix -- ping when ready. Also it looks like you need a require on projectile.

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label May 21, 2019
@alphapapa
Copy link
Contributor

@a13

  1. Why are you submitting the package instead of its author?
  2. If the package is intended to be used for more than just eshell, it should probably be refactored and renamed, with eshell being one of the prominent example use cases.

@purcell
Copy link
Member

purcell commented Jun 3, 2019

Thanks @alphapapa , I second those questions.

I also spotted that condition-case is being used to call projectile-project-root but silently fail if projectile is not installed, which is very hacky! You should usually consider using fboundp to check for its availability instead. (Also note that ignore-errors is a neater way of writing that condition-case.) But the best solution here is to remove the eshell-toggle-use-projectile-root custom var and instead have one called eshell-toggle-project-root-function, which defaults to 'ignore (i.e. always return nil.) Then users can set it to projectile-project-root if they want, and you don't have to have any references to projectile in your code.

@a13
Copy link
Contributor Author

a13 commented Jun 3, 2019

@alphapapa thanks!

  1. I don't think the original author cares about the package as much as I do these days (see the history) :), at the same time I see no reason to make a fork.

  2. any suggestions about naming?

@purcell thanks for your comment, I'm not the author of the projectile-related code there, but I'll rewrite it when I'll have more spare time.

@alphapapa
Copy link
Contributor

I don't think the original author cares about the package as much as I do these days (see the history) :), at the same time I see no reason to make a fork.

If the author doesn't care about the package, then it shouldn't be on MELPA, because a) if it's not maintained, it may not work well; and b) if the author does push changes, he may do so without regard for MELPA users. So if you do care about it, you should probably fork it, and then you could be the maintainer, and then you could submit it to MELPA as the maintainer.

any suggestions about naming?

If you plan to refactor and rename it, go ahead and refactor it, and worry about the name later. One will probably come to you as you refactor it. :)

@purcell
Copy link
Member

purcell commented Jun 4, 2019

The author is still committing their own and contributed changes, so I don't think there's any need for forking or renaming unless @a13 plans to strike out alone on a new path. Let's go ahead and merge this, and @a13 can submit a fix for the issues I mentioned as and when ready. :-)

@purcell purcell merged commit b7a3cf4 into melpa:master Jun 4, 2019
@a13
Copy link
Contributor Author

a13 commented Jun 4, 2019

@purcell Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-upstream Awaiting action from an upstream maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants