search

elixir-editors / emacs-elixir

Emacs major mode for Elixir
448 stars 94 forks source link

Remove pkg-info dependency #482

Closed phikal closed 3 years ago

phikal commented 3 years ago

Hi,

I am currently working on adding elixir-mode to NonGNU ELPA, but would like to avoid adding pkg-info. In case you are also interested, the following patch would remove the dependency.

phikal commented 3 years ago

If this patch is merged, it would also be necessary to bump the version tag to a greater value.

jsmestad commented 3 years ago

I have not had time to ensure the new code performs the same as pkg-info but I am ok with the change in theory 👍

phikal commented 3 years ago

In principle, it should be just as efficient, perhaps with the slight overhead of not having the load pkg-info. Either way, it is insignificant.

FYI: The last force-push fixes an issue I just noticed when byte-compiling. It should work now it all cases. Sadly it seems I cannot byte-compile the version name, otherwise any overhead would totally disappear, post-installation.

phikal commented 3 years ago

(ping?)

victorolinasc commented 3 years ago

Sorry for the long delay in looking into this. I will take a look at this next week for sure. I know this is ultra sub-optimal but I'll see to it.

This seems pretty straight and easy but I wanted to see how others are doing it. I've found out that magit makes some extra checks if running it from local version control and mostly use the lm-* functions instead of custom regexes for the headers. editorconfig seems to have a pretty simple implementation too. projectile makes no other assumption than just calling the lm-version function.

Maybe simplifying it even further might be the best way here? Wdyt?

Sorry if this is a very noob question... I am not 100% well versed on this unfortunately and that is why I wanted to take a deeper look into this.

phikal commented 3 years ago

There is no need to hurry, I just wanted to check if the issue went stale.

Maybe simplifying it even further might be the best way here? Wdyt?

It would be possible, of course. I initially used lm-header, but decided to reimplement it manually to avoid adding another dependency. lisp-mnt is a bit more generic, but this code should do the same job, because we know how the "Version" attribute looks like.

victorolinasc commented 3 years ago

Sorry but I thought that lisp-mnt is built-in in all Emacs versions we support. Is that not right? I may be missing something here but I think we can count that those functions will always be available where elixir-mode is running. Again, I might be wrong here for my Elisp noobness and please do correct me if anything I am assuming here is wrong. I really am not an expert in the subject here.

phikal commented 3 years ago

Victor Oliveira Nascimento @.***> writes:

Sorry but I thought that lisp-mnt is built-in in all Emacs versions we support. Is that not right?

That is true, but it is not loaded by default. Some people avoid loading libraries, even if they are built-in, but if you say elixir-mode doesn't care, I'll update the patch without any issue.

I may be missing something here but I think we can count that those functions will always be available where elixir-mode is running. Again, I might be wrong here for my Elisp noobness and please do correct me if anything I am assuming here is wrong. I really am not an expert in the subject here.

-- Philip Kaludercic

victorolinasc commented 3 years ago

Nice! I think we don't care about loading a built-in library. Actually I'd prefer not having custom code here for something that is built-in although not loaded by default.

Wdyt @jsmestad @axelson @Trevoke ?

phikal commented 3 years ago

The last version uses lisp-mnt (thought at compile time) and look more like the first suggestion, except that it handles byte compilation properly since it checks byte-compile-current-file.

jsmestad commented 3 years ago

@victorolinasc agreed. Nothing stopping us from changing our minds if a reason comes up in the future.

phikal commented 3 years ago

Nothing stopping us from changing our minds if a reason comes up in the future.

Well, the motivation behind this change is to add elixir-mode to NonGNU ELPA. If added, and this change were to be reverted, elixir-mode couldn't be installed via NonGNU ELPA anymore.

jsmestad commented 3 years ago

Nothing stopping us from changing our minds if a reason comes up in the future.

Well, the motivation behind this change is to add elixir-mode to NonGNU ELPA. If added, and this change were to be reverted, elixir-mode couldn't be installed via NonGNU ELPA anymore.

Correct. I am not suggesting we would revert it, just purely coming from a semver/agile perspective. This isn't a change we can't undo if it turns out we want/need to approach the problem differently. In the unlikely event such a situation presents itself.

victorolinasc commented 3 years ago

Hi @phikal ! I've taken a look once again and it looks fine. I'd just ask to move the version definition per se to a defconst just because I think it communicates best. Other than that I think it is nice :)

Thanks once again for your contribution :)

phikal commented 3 years ago

Done.

After merging, all I would ask for is to bump this now much discussed version tag, so that NonGNU ELPA will be able to detect a new release without pkg-info.

phikal commented 3 years ago

There is no inherent hurry right now, I am still waiting for a few other packages before I'll submit elixir-mode.

victorolinasc commented 3 years ago

Sorry it took so long =/

Just bumped to 2.4.0 :) If you need anything more to publish this under non-GNU Elpa just open an issue :)