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

Remove pkg-info dependency #482

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Conversation

phikal
Copy link
Contributor

@phikal phikal commented Aug 27, 2021

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
Copy link
Contributor Author

phikal commented Sep 18, 2021

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

@jsmestad
Copy link
Contributor

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
Copy link
Contributor Author

phikal commented Sep 18, 2021

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
Copy link
Contributor Author

phikal commented Sep 27, 2021

(ping?)

@victorolinasc
Copy link
Contributor

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
Copy link
Contributor Author

phikal commented Oct 1, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

phikal commented Oct 1, 2021 via email

@victorolinasc
Copy link
Contributor

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
Copy link
Contributor Author

phikal commented Oct 1, 2021

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
Copy link
Contributor

jsmestad commented Oct 1, 2021

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

@phikal
Copy link
Contributor Author

phikal commented Oct 1, 2021

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
Copy link
Contributor

jsmestad commented Oct 2, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

phikal commented Oct 4, 2021

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.

Copy link
Contributor

@victorolinasc victorolinasc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @phikal ! I will bump it to 2.5.0. Just waiting for answers on other PRs but I think this wouldn't hurt :)

@victorolinasc victorolinasc merged commit 50ac869 into elixir-editors:master Oct 5, 2021
@phikal
Copy link
Contributor Author

phikal commented Oct 5, 2021

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

@victorolinasc
Copy link
Contributor

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 :)

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

Successfully merging this pull request may close these issues.

3 participants