dash-docs-el / helm-dash

Browse Dash docsets inside emacs
510 stars 59 forks source link

Split dash-docs specific logic into separate library #173

Closed gilbertw1 closed 5 years ago

gilbertw1 commented 6 years ago

It would be great to have the hard work put into this package to interact with Dash docsets available to other packages without requiring any dependencies on helm. I personally use counsel-dash in my configuration which depends on helm-dash and as a result requires that I load helm as well. This would additionally encourage other packages to be created that can leverage dash docs as well.

To this end I've got a work in progress implementation of this separate library along with what I think is a smooth transition path forward for both helm-dash and counsel-dash. I've created a separate project for an elisp package called dash-docs. I created the dash-docs repo using the helm-dash repo to retain full git history since most of the functionality remains unchanged apart from being renamed. I'll also mention, that if this proposal is accepted I will be happy to transfer ownsership of this repository.

This new package dash-docs.el contains most of the dash specific code and both helm-dash.el and counsel-dash.el will depend on this common package. I've also updated both packages to maintain their current user interface requiring no changes to downstream users and simply use this new package to drive all of the dash specific logic.

Updated Repositories:

Proposed Migration Strategy:

Things Left to Do:

  1. Create new README.md in dash-docs repo.
  2. Verify tests, perform additional testing, minor cleanup.
  3. Verify each step in "Proposed Migration Strategy" will not break anything for existing users.

Sorry for the long issue. Please let me know what you think about this proposal and if you'd like me to change anything about it or if you see any problems proceeding this way.

Thanks!

kidd commented 6 years ago

Wow! That's super nice. First of all, thanks for your initiative and effort there!

I'd say it's a clear yes. @areina wdyt? I'm going to look at the code this week but the idea seems pretty sane

areina commented 6 years ago

Hey @gilbertw1, that's fantastic. I will also take a look at the code, but I like the idea. Let's talk during the next days to see how we can follow the plan that you have described. Thank you!

gilbertw1 commented 6 years ago

That's great, thanks for the quick response!

I was thinking about the best way to move forward with the new repo dash-docs or whatever we decide to call it. I think the cleanest approach would be to create a new repo which is essentially a clone of helm-dash and I could create a pull request against it with all of my changes specific to the new package. This would likely be the easiest way to do a code review and track the changes.

All the other changes could be independently evaluated in PRs against the two other projects (helm-dash and counsel-dash).

I think this division would make all the changes on the whole easier to track and evaluate.

jbaum98 commented 5 years ago

What's the status of this? Is there anything I can do to help out?

kidd commented 5 years ago

Early when this PR came I tested it and it worked fine, but I guess we should have more testing for it until we can do all the merges.

@gilbertw1 did an amazing job and we totally should get it merged. Let's go for it.

@jbaum98 would you be able to test one of them? helm? counsel?

If anyone happens to read this, it'd be nice if you comment and pick one of the 2 versions to try. Let's see how it goes.

I'll try to use the helm one.

mnewt commented 5 years ago

I've been subscribed to this issue and using counsel-dash for months. Specifically, I'm requiring these repos:

And using the commands:

It's awesome and I haven't had any problems at all.

gilbertw1 commented 5 years ago

Hey guys, sorry I got pretty busy and dropped the ball on following up with this. I've been using the repos as well in my personal configs with no issues so far.

I'll go through and update the Readme documentation of the dash-docs repo to match the new repo and the changes I made to it.

kidd commented 5 years ago

np! I also forgot about this, sorry... Hopefully we're handing it over by our 400stars :).

No rush, really.

Thanks again for your contributions

gilbertw1 commented 5 years ago

Alright, I've updated the Readme on the dash-docs repo to reflect the fact that it's no longer helm-dash and provide basic documentation on how to use it as a library.

Additionally, I've fixed up the tests so they all run properly now in dash-docs and removed the test related files from the helm-dash repo since they would be redundant there.

The next step that makes sense to me is to hand over the dash-docs repo to you guys, then we can get it submitted to MELPA. Once that is complete, then we can see about testing and merging my helm-dash specific changes into this repo, so that it will start using the new dash-docs package underneath.

kidd commented 5 years ago

We're fine with giving it to you if you are up to it. Maintenance is low on this project at this stage.

I'd be happy to handle it myself if you're busy, but it makes sense you have the opportunity to step up.

@areina is kinda MIA lately so I guess he's ok with this. :)

gilbertw1 commented 5 years ago

Sure, I'd be willing to maintain the project and handle the transition.

kidd commented 5 years ago

I'm trying it now and it works wonderfully. Will be trying it for the next few days, trying to stress it a bit more.

kidd commented 5 years ago

cc'ing some third parties that should probably be aware of this:

@syl20bnr (spacemacs author), we're not there yet, but when we do the change, I guess some changes in spacemacs should be applied (at least in the docs).

@nathankot (counsel-dash author), would you be ok with giving the package name to those new packages?

gilbertw1 commented 5 years ago

I've merged in the most recent changes into the dash-docs repository. How's the testing going?

kidd commented 5 years ago

Cool!

All good so far. I think we can proceed with the migration.

Is dash-docs published to melpa? I think we can go for that.

Also, we can prepare the PR to melpa to change helm-dash to point to your repo (notifying it should wait there a bit to be merged)

gilbertw1 commented 5 years ago

Sounds like a plan! I'll go ahead and submit dash-docs to melpa to get the ball rolling.

gilbertw1 commented 5 years ago

dash-docs has been submitted to melpa!

kidd commented 5 years ago

Ok, now we have dash-docs into melpa already.

Next step is taking over helm-dash and counsel-dash, right?

Just a heads up I found some variables and functions that were not correctly exported/imported, @gilbertw1 Could you review this PR https://github.com/gilbertw1/helm-dash/pull/1?

gilbertw1 commented 5 years ago

@kidd Awesome news!

I just got back from a vacation, I'll review and merge your PR shortly. Next step would be to submit a followup PR to Melpa to point to the new helm-dash repo. After that I'll work with counsel-dash to get that project updated as well.

Regarding the transition, do you think it'd be better move the repository which would retain all PR's and issues, or just start clean with the fork?

kidd commented 5 years ago

I hadn't thought there's the option of transferring the repo. Maybe it's the most sensible thing to do (maybe not a huge win, but some little win is already something),

I'll reach @areina and get this sorted ASAP.

Cheers!

areina commented 5 years ago

Hello,

sorry folks for not being very responsive. I can't transfer the repo to @gilbertw1 because he already has a fork with the same name.

There are two options:

One question @gilbertw1: What is your plan?: merging the different "completion frontends" into the dash-docs repo or keeping helm-, counsel- ... repositories? The former option could fit better with the idea of creating an organization, we could try to move everything there. If you plan to merge everything in one single repo, I think creating the org doesn't make sense.

Cheers

gilbertw1 commented 5 years ago

@areina, I think the best option here would be to create an organization to move the repository to.

I'm planning on keeping each project separate, so that makes the most sense. Additionally, this would prevent the need for any future movement if maintainers change.

I'm currently mobile, but I'll set up an organization shortly. Afterwards we can consolidate the repositories there.

Cheers!

areina commented 5 years ago

Cool, let me know when the organization is created and I will transfer this repo. Thank you for managing this, we really appreciate your effort.

gilbertw1 commented 5 years ago

No problem! I've benefited a lot from using helm/counsel-dash over the years, so I'm glad I can help evolve the project.

I've created the organization here: https://github.com/dash-docs-el

areina commented 5 years ago

I've just transferred the repo to the org. Would you mind adding @kidd as part of the org?

gilbertw1 commented 5 years ago

Yep, I sent him a request when I created it.

gilbertw1 commented 5 years ago

Hey guys, sorry for the delay. I've merged my fork into master and helm-dash is now backed by dash-docs!

I've built and tested the melpa package locally and everything went smoothly. I'll follow up once the update is available via Melpa and verify that as well. Once I've done that, I'll follow up with counel-dash.

kidd commented 5 years ago

great news!

Shout if you need some help with testing/anything

gilbertw1 commented 5 years ago

All right guys, I've verified that both the helm-dash and counsel-dash packages function properly post migration. I'm currently following up with counsel-dash now to remove the dependency on helm-dash.

Thanks for your help, I'm marking this issue as closed :)

syl20bnr commented 5 years ago

Nice work to everybody involved. PR welcome to update Spacemacs if any of you are willing to do it. Thank you!

gilbertw1 commented 5 years ago

@syl20bnr Sorry for the delay. I've created syl20bnr/spacemacs#12312 to handle any changes needed for spacemacs.

Thanks!