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

Melpa #31

Closed
jojojames opened this issue Nov 14, 2017 · 28 comments
Closed

Melpa #31

jojojames opened this issue Nov 14, 2017 · 28 comments
Labels

Comments

@jojojames
Copy link
Collaborator

@Ambrevar Lets try and move forward with Melpa so that it's easier to obtain evil-collection.

Here's a list of issues tagged with Melpa already. I am splitting it up by what I don't see as required and what is probably required.

Required
Normalize the documentation header - #12
Documentation: use "state", not "mode" - #13
Documentation: Review readme.org - #14
Customizable initial/primary state - #21

Not Required
Keybinding Syntax - #8
Rationale: Sorting vs. Filtering - #20
image-mode and pdf-tools conflict - #23
Rationale: Marking - #19

Some of the required are already done and are left as a reminder. The biggest one seems to be #21.

In not required, I don't want rationale discussions to block pushing to melpa and image-mode/pdf-mode also doesn't seem to be too important to block.

#8, keybinding syntax, I see it the same way I see evilify macro. We want to move in one direction but the current stuff is fine as it is too, so it shouldn't be a blocker.

Let me know if you disagree with any points. I'm trying to avoid spinning wheels here and leaving this repo "melpa-less" for months. I know quite a few people that don't like to vendor / use git submodules and would prefer going through the package archives.

@fbergroth
Copy link
Contributor

Not sure if it will be an issue, or the proper way to solve it, but the packages which contains bindings to a third party library will fail to byte compile.

@jojojames
Copy link
Collaborator Author

Probably due to the require statements. We might have to require with no errors.

@fbergroth
Copy link
Contributor

You could do that, but then you'd end up with warnings unless you forward declare variables.

@jojojames
Copy link
Collaborator Author

@fbergroth Which files do you think are problems?

Most files are just using function symbols (not variables) so I don't see a problem byte compiling them with the optional package require.

Try removing ag package for example and doing an optional require on it. It should byte compile fine.

I think, for the general case, that approach should work fine. Of course, if someone else had a better idea, we can go with that.

@fbergroth
Copy link
Contributor

I tested with vlf, and when I changed (require 'vlf nil t) I got this warning:

In evil-vlf-setup:
evil-vlf.el:36:4:Warning: reference to free variable ‘vlf-mode-map’

Which can be silenced by forward declaring it.

I'm not sure why I'm not getting the same warning with ag though.

@fbergroth
Copy link
Contributor

Have a look at the output of #32

@fbergroth
Copy link
Contributor

fbergroth commented Nov 16, 2017

With compilation warnings out of the way, there are some issues reported by package-lint. I fixed a couple in #35.

What remains is:

  • bad naming in evil-collection-util.el and evil-evilified-state.el
  • package-lint crashes in evil-pdf, on the line with (kbd "C-c <tab>") - but that's an issue with package-lint. That's why the last output of make lint is Wrong type argument: number-or-marker-p, tab.

Edit: added workaround for the package-lint crash. Now I'm also seeing

In ‘evil-term.el’:
  at 86:15: warning: This key sequence is reserved (see Key Binding Conventions in the Emacs Lisp manual)

It would be nice if make lint ran cleanly as well, so travis could run it.

@jojojames
Copy link
Collaborator Author

@fbergroth I've made some changes for those. Package-lint should be good now.

@jojojames
Copy link
Collaborator Author

melpa/melpa#5124

@Ambrevar
Copy link
Collaborator

Sorry for coming too late to the party, but I'd like to close one issue before release: stabilize the rationale, i.e. all "Rationale:" items.
I think it's important for the end users that our default keybindings do not change over time, or at least as little as possible.

I don't think it's so hard: I'll review the issues in depth in the coming days and make sure there is no conflict and everything is consistent. If we all agree with the result, then let's go MELPA.

What do you think?

@jojojames
Copy link
Collaborator Author

I want to keep the rationale more flexible. If we pour cement all over it right now, that would be unfortunate.

I'd rather we take a "we're releasing this package but it's still subject to change" approach. As this project matures, a lot of the patterns will harden naturally.

Also, it's already submitted to Melpa and awaiting approval.

@Ambrevar
Copy link
Collaborator

OK, we can go for "flexible".
I suggest we have a last thorough round check for consistency though: if the bindings happen to be a mess and inconsistent with the rationale, the package would gather poor reputation.

@jojojames
Copy link
Collaborator Author

We should just put big red warnings up top that the keybindings are still in a state of flux. (This would have to go in the commentary section of evil-collection.el so MELPA users can see the note.)

@jojojames
Copy link
Collaborator Author

@Ambrevar

As @purcell mentioned, we should prefix our sub-packages with 'evil-collection-'.

melpa/melpa#5124

I'll get to that at some point but if you had time and got to it before me, that'd be awesome too.

@Ambrevar
Copy link
Collaborator

Ambrevar commented Dec 3, 2017

@jojojames I can do the change now, i.e. prefix everything with evil-collection-.
I'll be waiting for your confirmation after the last comments on the MELPA thread.

@jojojames
Copy link
Collaborator Author

jojojames commented Dec 3, 2017

@Ambrevar I like @purcell's suggestion.

evilcol- ?

The package can still be evil-collection, but the namespacing inside can be shortened a little.

I'll leave it up to you though when you make the change. (evil-collection-) is fine too.

EDIT: If it wasn't clear. :) Make the change as you please.

@Ambrevar
Copy link
Collaborator

Ambrevar commented Dec 3, 2017

One last question before I apply the changes: should I rename all the files, the provides and the setup in evil-collection.el (which will be renamed to evilcol.el)?

That will break everyone's config.

@jojojames
Copy link
Collaborator Author

Yeah.

@jojojames
Copy link
Collaborator Author

@Ambrevar Let me know if you want me to make the change. Should be fairly quick. :)

@Ambrevar
Copy link
Collaborator

Ambrevar commented Dec 6, 2017

Sorry, I cannot commit much now I'm afraid. If you can do it, you should go ahead :)
Don't forget to update the readme and the header comments in evil-collection.el.

@jojojames
Copy link
Collaborator Author

Thanks. No worries. I'll go ahead and get to it at some point.

@jojojames
Copy link
Collaborator Author

Gave it best efforts to update the namespace. Lets fix things as we find them.

I ended up with evil-collection as looking at evilcol- was a little weird after I made the change. We get the added benefit of not breaking anybody's config for the simple case where they just call the init function.

@Ambrevar
Copy link
Collaborator

Ambrevar commented Dec 7, 2017

Good job, thank you for going through this!

There are a few issues with modes defining custom functions like in Eshell and EMMS. I'm working on it.

@jojojames
Copy link
Collaborator Author

Yeah, not surprised I broke a few things. :)

@Ambrevar
Copy link
Collaborator

Ambrevar commented Dec 7, 2017

Everything looks good on my end now.

@purcell
Copy link

purcell commented Dec 23, 2017

Merged into MELPA now - thanks! :-)

@jojojames
Copy link
Collaborator Author

Thanks again @purcell !

@Ambrevar
Copy link
Collaborator

Thanks @purcell!

I just posted an announcement on reddit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants